[AMDGPU] Fix operand definitions for atomic scalar memory instructions. #71799
Conversation
CPol and CPol_GLC1 operand classes have identical predicates, which means AsmParser cannot differentiate between the RTN and non-RTN variants of the instructions. When it currently selects the wrong instruction, a hack in cvtSMEMAtomic() corrects the op-code. Using the new predicated-value operands makes this hack and the whole conversion function not needed. Other uses of CPol_GLC1 operands are to be addressed separately. Resolves about half of the remaining ~1000 pairs of ambiguous instructions. Part of <llvm#69256>.
|
@llvm/pr-subscribers-backend-amdgpu Author: Ivan Kosarev (kosarev) ChangesCPol and CPol_GLC1 operand classes have identical predicates, which means AsmParser cannot differentiate between the RTN and non-RTN variants of the instructions. When it currently selects the wrong instruction, a hack in cvtSMEMAtomic() corrects the op-code. Using the new predicated-value operands makes this hack and the whole conversion function not needed. Other uses of CPol_GLC1 operands are to be addressed separately. Resolves about half of the remaining ~1000 pairs of ambiguous instructions. Part of <#69256>. Full diff: https://github.com/llvm/llvm-project/pull/71799.diff 4 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
index 23242ad84b0c425..d2d09a0b1fc5485 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
@@ -165,6 +165,20 @@ class ImmOperand<ValueType type, string name = NAME, bit optional = 0,
def s16imm : ImmOperand<i16, "S16Imm", 0, "printU16ImmOperand">;
def u16imm : ImmOperand<i16, "U16Imm", 0, "printU16ImmOperand">;
+class ValuePredicatedOperand<CustomOperand op, string valuePredicate,
+ bit optional = 0>
+ : CustomOperand<op.Type, optional> {
+ let ImmTy = op.ImmTy;
+ defvar OpPredicate = op.ParserMatchClass.PredicateMethod;
+ let PredicateMethod =
+ "getPredicate([](const AMDGPUOperand &Op) -> bool { "#
+ "return Op."#OpPredicate#"() && "#valuePredicate#"; })";
+ let ParserMethod = op.ParserMatchClass.ParserMethod;
+ let DefaultValue = op.DefaultValue;
+ let DefaultMethod = op.DefaultMethod;
+ let PrintMethod = op.PrintMethod;
+}
+
//===--------------------------------------------------------------------===//
// Custom Operands
//===--------------------------------------------------------------------===//
diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index fe2729f9790aa7c..be74c627d213756 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -909,6 +909,10 @@ class AMDGPUOperand : public MCParsedAsmOperand {
bool isWaitVDST() const;
bool isWaitEXP() const;
+ auto getPredicate(std::function<bool(const AMDGPUOperand &Op)> P) const {
+ return std::bind(P, *this);
+ }
+
StringRef getToken() const {
assert(isToken());
return StringRef(Tok.Data, Tok.Length);
@@ -1773,7 +1777,6 @@ class AMDGPUAsmParser : public MCTargetAsmParser {
void cvtVOP3Interp(MCInst &Inst, const OperandVector &Operands);
void cvtVINTERP(MCInst &Inst, const OperandVector &Operands);
- void cvtSMEMAtomic(MCInst &Inst, const OperandVector &Operands);
bool parseDimId(unsigned &Encoding);
ParseStatus parseDim(OperandVector &Operands);
@@ -7722,66 +7725,6 @@ void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyCPol, 0);
}
-//===----------------------------------------------------------------------===//
-// SMEM
-//===----------------------------------------------------------------------===//
-
-void AMDGPUAsmParser::cvtSMEMAtomic(MCInst &Inst, const OperandVector &Operands) {
- OptionalImmIndexMap OptionalIdx;
- bool IsAtomicReturn = false;
-
- for (unsigned i = 1, e = Operands.size(); i != e; ++i) {
- AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);
- if (!Op.isCPol())
- continue;
- IsAtomicReturn = Op.getImm() & AMDGPU::CPol::GLC;
- break;
- }
-
- if (!IsAtomicReturn) {
- int NewOpc = AMDGPU::getAtomicNoRetOp(Inst.getOpcode());
- if (NewOpc != -1)
- Inst.setOpcode(NewOpc);
- }
-
- IsAtomicReturn = MII.get(Inst.getOpcode()).TSFlags &
- SIInstrFlags::IsAtomicRet;
-
- for (unsigned i = 1, e = Operands.size(); i != e; ++i) {
- AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);
-
- // Add the register arguments
- if (Op.isReg()) {
- Op.addRegOperands(Inst, 1);
- if (IsAtomicReturn && i == 1)
- Op.addRegOperands(Inst, 1);
- continue;
- }
-
- // Handle the case where soffset is an immediate
- if (Op.isImm() && Op.getImmTy() == AMDGPUOperand::ImmTyNone) {
- Op.addImmOperands(Inst, 1);
- continue;
- }
-
- // Handle tokens like 'offen' which are sometimes hard-coded into the
- // asm string. There are no MCInst operands for these.
- if (Op.isToken()) {
- continue;
- }
- assert(Op.isImm());
-
- // Handle optional arguments
- OptionalIdx[Op.getImmTy()] = i;
- }
-
- if ((int)Inst.getNumOperands() <=
- AMDGPU::getNamedOperandIdx(Inst.getOpcode(), AMDGPU::OpName::offset))
- addOptionalImmOperand(Inst, Operands, OptionalIdx,
- AMDGPUOperand::ImmTySMEMOffsetMod);
- addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyCPol, 0);
-}
-
//===----------------------------------------------------------------------===//
// smrd
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.td b/llvm/lib/Target/AMDGPU/SIInstrInfo.td
index b0493edfa335acf..02c769bf21ac3ea 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.td
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.td
@@ -1078,6 +1078,8 @@ def highmod : NamedBitOperand<"high", "High">;
def CPol : CustomOperand<i32, 1>;
def CPol_0 : DefaultOperand<CPol, 0>;
def CPol_GLC1 : DefaultOperand<CPol, 1>;
+def CPol_GLC : ValuePredicatedOperand<CPol, "Op.getImm() & CPol::GLC">;
+def CPol_NonGLC : ValuePredicatedOperand<CPol, "!(Op.getImm() & CPol::GLC)", 1>;
def TFE : NamedBitOperand<"tfe">;
def UNorm : NamedBitOperand<"unorm">;
diff --git a/llvm/lib/Target/AMDGPU/SMInstructions.td b/llvm/lib/Target/AMDGPU/SMInstructions.td
index 7ca685a0cc5d5e1..6235965b6e165bc 100644
--- a/llvm/lib/Target/AMDGPU/SMInstructions.td
+++ b/llvm/lib/Target/AMDGPU/SMInstructions.td
@@ -234,8 +234,6 @@ class SM_Atomic_Pseudo <string opName,
let IsAtomicNoRet = !not(isRet);
let IsAtomicRet = isRet;
-
- let AsmMatchConverter = "cvtSMEMAtomic";
}
class SM_Pseudo_Atomic<string opName,
@@ -245,7 +243,7 @@ class SM_Pseudo_Atomic<string opName,
bit isRet,
string opNameWithSuffix =
opName # offsets.Variant # !if(isRet, "_RTN", ""),
- Operand CPolTy = !if(isRet, CPol_GLC1, CPol)> :
+ Operand CPolTy = !if(isRet, CPol_GLC, CPol_NonGLC)> :
SM_Atomic_Pseudo<opName,
!if(isRet, (outs dataClass:$sdst), (outs)),
!con((ins dataClass:$sdata, baseClass:$sbase), offsets.Ins,
|
Nope. Same as with the changes fixing subtarget predicates, this removes the dependency on the order AsmMatcherEmitter happens to match instructions in, so no easy ways to test this, it seems. |
| @@ -7722,66 +7725,6 @@ void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst, | |||
| addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyCPol, 0); | |||
| } | |||
|
|
|||
| //===----------------------------------------------------------------------===// | |||
| @@ -909,6 +909,10 @@ class AMDGPUOperand : public MCParsedAsmOperand { | |||
| bool isWaitVDST() const; | |||
| bool isWaitEXP() const; | |||
|
|
|||
| auto getPredicate(std::function<bool(const AMDGPUOperand &Op)> P) const { | |||
There was a problem hiding this comment.
Wouldn't it be simpler to implement a couple more named predicate methods in AMDGPUOperand, e.g.:
bool isGLC() { return getImm() & CPol::GLC; }
Anyway I don't object if you want to do it this way, but it took me a while to understand.
There was a problem hiding this comment.
I think I'd prefer to have a single generic helper and use that to avoid scattering details of operand definitions over multiple places. Ultimately, the idea is to have an interface to AsmParser that would allow implementing a new kind of operands with a couple lines in a single place in an intuitive manner and not being wrong thinking it does the expected thing.
There was a problem hiding this comment.
OK. In the long term do you think we should aim for a deisgn where all operand predicate strings in .td files are expressions involving Op (like Op.isCPol()) instead of just names of predicate methods (like isCPol)?
There was a problem hiding this comment.
I don't know. I see it more an implementation detail and would just do what seems most natural given the current state of things. My immediate impression is that getPredicate() basically works around AsmMatcherEmitter expecting the predicate be an operand method, which doesn't seem to work well for non-trivial cases.
Speaking more generally, I would try to design it in such a way that wouldn't require specifying any predicate names explicitly at all, whether it's with or without the object expression.
…s. (llvm#71799) CPol and CPol_GLC1 operand classes have identical predicates, which means AsmParser cannot differentiate between the RTN and non-RTN variants of the instructions. When it currently selects the wrong instruction, a hack in cvtSMEMAtomic() corrects the op-code. Using the new predicated-value operands makes this hack and the whole conversion function not needed. Other uses of CPol_GLC1 operands are to be addressed separately. Resolves about half of the remaining ~1000 pairs of ambiguous instructions. Part of <llvm#69256>.
CPol and CPol_GLC1 operand classes have identical predicates, which means AsmParser cannot differentiate between the RTN and non-RTN variants of the instructions. When it currently selects the wrong instruction, a hack in cvtSMEMAtomic() corrects the op-code. Using the new predicated-value operands makes this hack and the whole conversion function not needed.
Other uses of CPol_GLC1 operands are to be addressed separately.
Resolves about half of the remaining ~1000 pairs of ambiguous instructions.
Part of #69256.