Skip to content

[AMDGPU] Fix operand definitions for atomic scalar memory instructions. #71799

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 10, 2023
Merged

[AMDGPU] Fix operand definitions for atomic scalar memory instructions. #71799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
//===--------------------------------------------------------------------===//
Expand Down
65 changes: 4 additions & 61 deletions llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,10 @@ class AMDGPUOperand : public MCParsedAsmOperand {
bool isWaitVDST() const;
bool isWaitEXP() const;

auto getPredicate(std::function<bool(const AMDGPUOperand &Op)> P) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return std::bind(P, *this);
}

StringRef getToken() const {
assert(isToken());
return StringRef(Tok.Data, Tok.Length);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -7722,66 +7725,6 @@ void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyCPol, 0);
}

//===----------------------------------------------------------------------===//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

// 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
//===----------------------------------------------------------------------===//
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Target/AMDGPU/SIInstrInfo.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Expand Down
4 changes: 1 addition & 3 deletions llvm/lib/Target/AMDGPU/SMInstructions.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down