-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CodeGen] Add MO_LaneMask type and a new COPY_LANEMASK instruction #151944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-arm Author: Vikash Gupta (vg0204) ChangesThis patch adds a new MachineOperand type to represent the laneBitmask as MO_LaneMask that can be used in the instructions to represent the relevant information associated with the register operands of the same such as liveness. One such use case of this patch can be seen in #151123, where it can be used to store live regUnits information corresponding to the input register of the COPY instructions, later to be used this mask to expand the COPY as in accordance to different targets. Full diff: https://github.com/llvm/llvm-project/pull/151944.diff 12 Files Affected:
diff --git a/llvm/docs/MIRLangRef.rst b/llvm/docs/MIRLangRef.rst
index a505c1ea4b0aa..4e12239e0e68f 100644
--- a/llvm/docs/MIRLangRef.rst
+++ b/llvm/docs/MIRLangRef.rst
@@ -819,6 +819,7 @@ For an int eq predicate ``ICMP_EQ``, the syntax is:
.. TODO: Describe the syntax of the metadata machine operands, and the
instructions debug location attribute.
.. TODO: Describe the syntax of the register live out machine operands.
+.. TODO: Describe the syntax of the lanemask machine operands.
.. TODO: Describe the syntax of the machine memory operands.
Comments
diff --git a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
index e705d7d99544c..a07fea19a4785 100644
--- a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
+++ b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
@@ -292,6 +292,11 @@ class MachineInstrBuilder {
return *this;
}
+ const MachineInstrBuilder &addLaneMask(LaneBitmask LaneMask) const {
+ MI->addOperand(*MF, MachineOperand::CreateLaneMask(LaneMask));
+ return *this;
+ }
+
const MachineInstrBuilder &addSym(MCSymbol *Sym,
unsigned char TargetFlags = 0) const {
MI->addOperand(*MF, MachineOperand::CreateMCSymbol(Sym, TargetFlags));
diff --git a/llvm/include/llvm/CodeGen/MachineOperand.h b/llvm/include/llvm/CodeGen/MachineOperand.h
index 646588a2a92a5..0fd999364234b 100644
--- a/llvm/include/llvm/CodeGen/MachineOperand.h
+++ b/llvm/include/llvm/CodeGen/MachineOperand.h
@@ -16,6 +16,7 @@
#include "llvm/ADT/DenseMapInfo.h"
#include "llvm/CodeGen/Register.h"
#include "llvm/IR/Intrinsics.h"
+#include "llvm/MC/LaneBitmask.h"
#include "llvm/Support/Compiler.h"
#include <cassert>
@@ -69,7 +70,8 @@ class MachineOperand {
MO_Predicate, ///< Generic predicate for ISel
MO_ShuffleMask, ///< Other IR Constant for ISel (shuffle masks)
MO_DbgInstrRef, ///< Integer indices referring to an instruction+operand
- MO_Last = MO_DbgInstrRef
+ MO_LaneMask, ///< Mask to represent active parts of registers
+ MO_Last = MO_LaneMask
};
private:
@@ -178,6 +180,7 @@ class MachineOperand {
Intrinsic::ID IntrinsicID; // For MO_IntrinsicID.
unsigned Pred; // For MO_Predicate
ArrayRef<int> ShuffleMask; // For MO_ShuffleMask
+ LaneBitmask LaneMask; // For MO_LaneMask
struct { // For MO_Register.
// Register number is in SmallContents.RegNo.
@@ -360,6 +363,7 @@ class MachineOperand {
bool isIntrinsicID() const { return OpKind == MO_IntrinsicID; }
bool isPredicate() const { return OpKind == MO_Predicate; }
bool isShuffleMask() const { return OpKind == MO_ShuffleMask; }
+ bool isLaneMask() const { return OpKind == MO_LaneMask; }
//===--------------------------------------------------------------------===//
// Accessors for Register Operands
//===--------------------------------------------------------------------===//
@@ -624,6 +628,11 @@ class MachineOperand {
return Contents.ShuffleMask;
}
+ LaneBitmask getLaneMask() const {
+ assert(isLaneMask() && "Wrong MachineOperand accessor");
+ return Contents.LaneMask;
+ }
+
/// Return the offset from the symbol in this operand. This always returns 0
/// for ExternalSymbol operands.
int64_t getOffset() const {
@@ -989,6 +998,12 @@ class MachineOperand {
return Op;
}
+ static MachineOperand CreateLaneMask(LaneBitmask LaneMask) {
+ MachineOperand Op(MachineOperand::MO_LaneMask);
+ Op.Contents.LaneMask = LaneMask;
+ return Op;
+ }
+
friend class MachineInstr;
friend class MachineRegisterInfo;
diff --git a/llvm/lib/CodeGen/MIRParser/MILexer.cpp b/llvm/lib/CodeGen/MIRParser/MILexer.cpp
index 8b72c295416a2..bb714653d79dc 100644
--- a/llvm/lib/CodeGen/MIRParser/MILexer.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MILexer.cpp
@@ -266,6 +266,7 @@ static MIToken::TokenKind getIdentifierKind(StringRef Identifier) {
.Case("constant-pool", MIToken::kw_constant_pool)
.Case("call-entry", MIToken::kw_call_entry)
.Case("custom", MIToken::kw_custom)
+ .Case("lanemask", MIToken::kw_lanemask)
.Case("liveout", MIToken::kw_liveout)
.Case("landing-pad", MIToken::kw_landing_pad)
.Case("inlineasm-br-indirect-target",
diff --git a/llvm/lib/CodeGen/MIRParser/MILexer.h b/llvm/lib/CodeGen/MIRParser/MILexer.h
index 0627f176b9e00..8c4fb0e63895a 100644
--- a/llvm/lib/CodeGen/MIRParser/MILexer.h
+++ b/llvm/lib/CodeGen/MIRParser/MILexer.h
@@ -122,6 +122,7 @@ struct MIToken {
kw_constant_pool,
kw_call_entry,
kw_custom,
+ kw_lanemask,
kw_liveout,
kw_landing_pad,
kw_inlineasm_br_indirect_target,
diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index 6a464d9dd6886..c09734ab8f87c 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -496,6 +496,7 @@ class MIParser {
bool parseTargetIndexOperand(MachineOperand &Dest);
bool parseDbgInstrRefOperand(MachineOperand &Dest);
bool parseCustomRegisterMaskOperand(MachineOperand &Dest);
+ bool parseLaneMaskOperand(MachineOperand &Dest);
bool parseLiveoutRegisterMaskOperand(MachineOperand &Dest);
bool parseMachineOperand(const unsigned OpCode, const unsigned OpIdx,
MachineOperand &Dest,
@@ -2870,6 +2871,32 @@ bool MIParser::parseCustomRegisterMaskOperand(MachineOperand &Dest) {
return false;
}
+bool MIParser::parseLaneMaskOperand(MachineOperand &Dest) {
+ assert(Token.is(MIToken::kw_lanemask));
+
+ lex();
+ if (expectAndConsume(MIToken::lparen))
+ return error("expected syntax lanemask(...)");
+
+ LaneBitmask LaneMask = LaneBitmask::getAll();
+ // Parse lanemask.
+ if (Token.isNot(MIToken::IntegerLiteral) && Token.isNot(MIToken::HexLiteral))
+ return error("expected a lane mask");
+ static_assert(sizeof(LaneBitmask::Type) == sizeof(uint64_t),
+ "Use correct get-function for lane mask");
+ LaneBitmask::Type V;
+ if (getUint64(V))
+ return error("invalid lanemask value");
+ LaneMask = LaneBitmask(V);
+ lex();
+
+ if (expectAndConsume(MIToken::rparen))
+ return error("lanemask should be terminated by ')'.");
+
+ Dest = MachineOperand::CreateLaneMask(LaneMask);
+ return false;
+}
+
bool MIParser::parseLiveoutRegisterMaskOperand(MachineOperand &Dest) {
assert(Token.is(MIToken::kw_liveout));
uint32_t *Mask = MF.allocateRegMask();
@@ -2970,6 +2997,8 @@ bool MIParser::parseMachineOperand(const unsigned OpCode, const unsigned OpIdx,
return parseIntrinsicOperand(Dest);
case MIToken::kw_target_index:
return parseTargetIndexOperand(Dest);
+ case MIToken::kw_lanemask:
+ return parseLaneMaskOperand(Dest);
case MIToken::kw_liveout:
return parseLiveoutRegisterMaskOperand(Dest);
case MIToken::kw_floatpred:
diff --git a/llvm/lib/CodeGen/MIRPrinter.cpp b/llvm/lib/CodeGen/MIRPrinter.cpp
index ce1834a90ca54..3945acbb505fe 100644
--- a/llvm/lib/CodeGen/MIRPrinter.cpp
+++ b/llvm/lib/CodeGen/MIRPrinter.cpp
@@ -928,7 +928,8 @@ static void printMIOperand(raw_ostream &OS, MFPrintState &State,
case MachineOperand::MO_Predicate:
case MachineOperand::MO_BlockAddress:
case MachineOperand::MO_DbgInstrRef:
- case MachineOperand::MO_ShuffleMask: {
+ case MachineOperand::MO_ShuffleMask:
+ case MachineOperand::MO_LaneMask: {
unsigned TiedOperandIdx = 0;
if (ShouldPrintRegisterTies && Op.isReg() && Op.isTied() && !Op.isDef())
TiedOperandIdx = Op.getParent()->findTiedOperandIdx(OpIdx);
diff --git a/llvm/lib/CodeGen/MIRVRegNamerUtils.cpp b/llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
index a22cc91b90542..0afb53e1d8f2c 100644
--- a/llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
+++ b/llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
@@ -106,6 +106,7 @@ std::string VRegRenamer::getInstructionOpcodeHash(MachineInstr &MI) {
case MachineOperand::MO_ExternalSymbol:
case MachineOperand::MO_GlobalAddress:
case MachineOperand::MO_BlockAddress:
+ case MachineOperand::MO_LaneMask:
case MachineOperand::MO_RegisterMask:
case MachineOperand::MO_RegisterLiveOut:
case MachineOperand::MO_Metadata:
diff --git a/llvm/lib/CodeGen/MachineOperand.cpp b/llvm/lib/CodeGen/MachineOperand.cpp
index c612f8de7b50b..4d1ce8894e0d0 100644
--- a/llvm/lib/CodeGen/MachineOperand.cpp
+++ b/llvm/lib/CodeGen/MachineOperand.cpp
@@ -380,6 +380,8 @@ bool MachineOperand::isIdenticalTo(const MachineOperand &Other) const {
return getPredicate() == Other.getPredicate();
case MachineOperand::MO_ShuffleMask:
return getShuffleMask() == Other.getShuffleMask();
+ case MachineOperand::MO_LaneMask:
+ return getLaneMask() == Other.getLaneMask();
}
llvm_unreachable("Invalid machine operand type");
}
@@ -445,6 +447,9 @@ hash_code llvm::hash_value(const MachineOperand &MO) {
return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getPredicate());
case MachineOperand::MO_ShuffleMask:
return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getShuffleMask());
+ case MachineOperand::MO_LaneMask:
+ return hash_combine(MO.getType(), MO.getTargetFlags(),
+ MO.getLaneMask().getAsInteger());
}
llvm_unreachable("Invalid machine operand type");
}
@@ -1004,11 +1009,11 @@ void MachineOperand::print(raw_ostream &OS, ModuleSlotTracker &MST,
}
case MachineOperand::MO_Predicate: {
auto Pred = static_cast<CmpInst::Predicate>(getPredicate());
- OS << (CmpInst::isIntPredicate(Pred) ? "int" : "float") << "pred("
- << Pred << ')';
+ OS << (CmpInst::isIntPredicate(Pred) ? "int" : "float") << "pred(" << Pred
+ << ')';
break;
}
- case MachineOperand::MO_ShuffleMask:
+ case MachineOperand::MO_ShuffleMask: {
OS << "shufflemask(";
ArrayRef<int> Mask = getShuffleMask();
StringRef Separator;
@@ -1023,6 +1028,14 @@ void MachineOperand::print(raw_ostream &OS, ModuleSlotTracker &MST,
OS << ')';
break;
}
+ case MachineOperand::MO_LaneMask: {
+ OS << "lanemask(";
+ LaneBitmask LaneMask = getLaneMask();
+ OS << "0x" << PrintLaneMask(LaneMask);
+ OS << ')';
+ break;
+ }
+ }
}
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
diff --git a/llvm/lib/CodeGen/MachineStableHash.cpp b/llvm/lib/CodeGen/MachineStableHash.cpp
index 9d56696079478..70e66e712b8f1 100644
--- a/llvm/lib/CodeGen/MachineStableHash.cpp
+++ b/llvm/lib/CodeGen/MachineStableHash.cpp
@@ -164,6 +164,10 @@ stable_hash llvm::stableHashValue(const MachineOperand &MO) {
return stable_hash_combine(MO.getType(), MO.getTargetFlags(),
stable_hash_name(SymbolName));
}
+ case MachineOperand::MO_LaneMask: {
+ return stable_hash_combine(MO.getType(), MO.getTargetFlags(),
+ MO.getLaneMask().getAsInteger());
+ }
case MachineOperand::MO_CFIIndex:
return stable_hash_combine(MO.getType(), MO.getTargetFlags(),
MO.getCFIIndex());
diff --git a/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp b/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
index 57141ab69223f..0dc54b4f4aad1 100644
--- a/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
@@ -930,6 +930,7 @@ static bool IsAnAddressOperand(const MachineOperand &MO) {
return true;
case MachineOperand::MO_RegisterMask:
case MachineOperand::MO_RegisterLiveOut:
+ case MachineOperand::MO_LaneMask:
return false;
case MachineOperand::MO_Metadata:
case MachineOperand::MO_MCSymbol:
diff --git a/llvm/unittests/CodeGen/MachineOperandTest.cpp b/llvm/unittests/CodeGen/MachineOperandTest.cpp
index 3f3f48fcc7c58..dcd731e5cb896 100644
--- a/llvm/unittests/CodeGen/MachineOperandTest.cpp
+++ b/llvm/unittests/CodeGen/MachineOperandTest.cpp
@@ -288,6 +288,23 @@ TEST(MachineOperandTest, PrintGlobalAddress) {
}
}
+TEST(MachineOperandTest, PrintLaneMask) {
+ // Create a MachineOperand with a lanemask and print it.
+ LaneBitmask LaneMask = LaneBitmask(12);
+ MachineOperand MO = MachineOperand::CreateLaneMask(LaneMask);
+
+ // Checking some preconditions on the newly created
+ // MachineOperand.
+ ASSERT_TRUE(MO.isLaneMask());
+ ASSERT_TRUE(MO.getLaneMask() == LaneMask);
+
+ std::string str;
+ // Print a MachineOperand that is lanemask as in HEX representation.
+ raw_string_ostream OS(str);
+ MO.print(OS, /*TRI=*/nullptr);
+ ASSERT_TRUE(str == "lanemask(0x000000000000000C)");
+}
+
TEST(MachineOperandTest, PrintRegisterLiveOut) {
// Create a MachineOperand with a register live out list and print it.
uint32_t Mask = 0;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSERT_TRUE(MO.getLaneMask() == LaneMask); | |
EXPECT_EQ(MO.getLaneMask(), LaneMask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSERT_TRUE(str == "lanemask(0x000000000000000C)"); | |
EXPECT_EQ(str, "lanemask(0x000000000000000C)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should get MIR print/parse tests and cover the error cases too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same as you did for MO_ShuffleMask, but don't know with which instruction it should go with( not associated yet to any kind of MI), in order to write test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could introduce the COPY_LANEMASK instruction here I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a normal (generic) COPY, with additional machineOperand as lanemask. Any other specs I need to know about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a new opcode, not COPY but yes. It should also forbid subregister operands
1018bd1
to
1520a50
Compare
0afaaee
to
98ee227
Compare
llvm/test/CodeGen/MIR/AMDGPU/parse-lanemask-operand-invalid-1.mir
Outdated
Show resolved
Hide resolved
llvm/test/CodeGen/MIR/AMDGPU/parse-lanemask-operand-invalid-0.mir
Outdated
Show resolved
Hide resolved
d773d45
to
8b4def7
Compare
836d3bf
to
78fc32c
Compare
Please post an RFC on discourse explaining what problem this solves and how. |
My immediate desire for this is to stop using the poorly-defined COPY bundles that split kit uses if only some lanes are live. It isn't a standard bundle format with a leading BUNDLE header instruction, and also requires snippet copy identification to do more work to identify these bundles |
llvm/docs/MIRLangRef.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that lanemask operands can only be used on instructions with a single source register operand? Or would it be possible in the future to have $vgpr0 = SOME_OTHER_OP $vgpr1, $vgpr2, lanemask(0x42)
, in which case it applies to all register operands? Can an instruction have more than one lanemask operand? Please clarify this in the docs (and add more verifier checks if relevant)
I think you need to abstract this whole concept a bit more, since this document is target-independent, and many targets don't have hidden lanes like AMDGPU does. Are you planning to use these in any target-independent passes? If not, then we might get away with smth hand-wavy like "the interpretation of lanemasks is up to each target", but otherwise I think you'll need to be more specific about how this behaves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that lanemask operands can only be used on instructions with a single source register operand? Or would it be possible in the future to have $vgpr0 = SOME_OTHER_OP $vgpr1, $vgpr2, lanemask(0x42), in which case it applies to all register operands? Can an instruction have more than one lanemask operand? Please clarify this in the docs (and add more verifier checks if relevant)
Sure, will do!
I think you need to abstract this whole concept a bit more, since this document is target-independent, and many targets don't have hidden lanes like AMDGPU does. Are you planning to use these in any target-independent passes? If not, then we might get away with smth hand-wavy like "the interpretation of lanemasks is up to each target", but otherwise I think you'll need to be more specific about how this behaves.
This is needed for something that is target-independent, as the interpretation of lanemask that is is used in defining the MO_Lanemask is based on the lanemask defintion as defined by codegen for virtual registers. Even its use-case can be extended to different targets. Its different question how each target can utilize it for their benefit.
llvm/lib/CodeGen/MachineVerifier.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description in TargetOpcodes.def says this is a copy between physical registers, but this code suggests vregs are also allowed. Can you please clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out, need to update that. As when started that time made assumption for only PhysReg which eventually got changed to support both virtReg & PhysReg by COPY_LANEMASK opcode
I am in need of MO_Lanemask as well to encode liveness information for source registers as in case of COPY (or SPILL) instructions, that can be used for selective expansion of those parts of registers tuple which are defined(#151123 & #151124). So, should I still go with RFC explaining above point & the one given by Matt, along with its potential use-cases inf future. |
That would be good. |
Small style suggestion ... try rewriting the commit description without saying "this patch"? The first line of the commit description is unnecessarily long. I believe it could be shortened to the following with negligible loss of information:
Also, as an informed layman (but not familiar with backend jargon), this sentence didn't really tell me much:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated whitespace change?
My main concern with adding the new type of copy is making sure MachineInstr::getRegClassConstraint works correctly when a specific subclass is needed to represent the subregister copies. Right now with the expanded bundle, the subreg is applied when computing the effective class constraint but that will be hidden with the lane mask |
ed9cb68
to
333ab5d
Compare
Added the RFC : https://discourse.llvm.org/t/llvm-codegen-rfc-add-mo-lanemask-type-and-a-new-copy-lanemask-instruction/88021 Gentle ping : @arsenm , @jayfoad , @cdevadas , @ssahasra , @rovka , @qcolombet |
Gentle reminder!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the "cannot be only used" part mean? Please clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it!
This patch adds a new MachineOperand type to represent the laneBitmask as MO_LaneMask that can be used in the instructions to represent the relevant information associated with the register operands of the same such as liveness.
This new instruction takes laneMask as an operand with respect to the input operand storing the essential information.
bd0ec03
to
238f2a3
Compare
Gentle Ping for review! |
Introduce MO_LaneMask as new machine operand type. This can be used to hold liveness infomation at sub-register granularity for register-type operands. We also introduce a new COPY_LANEMASK instruction that uses MO_lanemask operand to perform partial copy from source register opernad.
One such use case of MO_LaneMask can be seen in #151123, where it can be used to store live regUnits information corresponding to the source register of the COPY instructions, later can be used during CopyPhysReg expansion.