[AMDGPU][MC] Incorrect parsing of flat/global atomic modifiers

[dp]

Summary

See bug 35730: https://bugs.llvm.org/show_bug.cgi?id=35730

[vpykhtin]

LGTM.

[artem.tamazov]

Please add negative tests: (a) VDST present but none GLC and (b) none VDST but GLC presents. Otherwise fine.

[dp]

Added a few negative tests and one positive.

Note that the following code is assembled ok, though 'glc' is omitted:

flat_atomic_cmpswap v0, v[1:2], v[3:4]

This is due to the way llvm handles optional operands. In this case tablegen-generated operand matcher tries to match (expected) 'offset' to an empty list of actual operands and failed doing that. However 'offset' is an optional operand so the match is considered successful.

This would not be easy to fix, and I think we should presume this a 'feature' :-)

[artem.tamazov]

In D41598#964823, @dp wrote:

Note that the following code is assembled ok, though 'glc' is omitted:

flat_atomic_cmpswap v0, v[1:2], v[3:4]

This would not be easy to fix, and I think we should presume this a 'feature' :-)

I think this is a low-priority bug because this does not comply to (6.2) from Bugzilla 35730. Let's update that Bugzllia but keep it open after this fix is submitted.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

switch (RegKind) {
       default: break;
     }
   }
 };
 
 class AMDGPUAsmParser : public MCTargetAsmParser {
   MCAsmParser &Parser;
 
+  // Number of extra operands parsed after the first optional operand.
+  // This may be necessary to skip hardcoded mandatory operands.
+  static const unsigned MAX_OPR_LOOKAHEAD = 1;
+
   unsigned ForcedEncodingSize = 0;
   bool ForcedDPP = false;
   bool ForcedSDWA = false;
   KernelScopeInfo KernelScope;
 
   /// @name Auto-generated Match Functions
   /// {
 
private:
   bool trySkipId(const StringRef Id);
   bool trySkipToken(const AsmToken::TokenKind Kind);
   bool skipToken(const AsmToken::TokenKind Kind, const StringRef ErrMsg);
   bool parseString(StringRef &Val, const StringRef ErrMsg = "expected a string");
   bool parseExpr(int64_t &Imm);
 
 public:
   OperandMatchResultTy parseOptionalOperand(OperandVector &Operands);
+  OperandMatchResultTy parseOptionalOpr(OperandVector &Operands);
 
   OperandMatchResultTy parseExpTgt(OperandVector &Operands);
   OperandMatchResultTy parseSendMsgOp(OperandVector &Operands);
   OperandMatchResultTy parseInterpSlot(OperandVector &Operands);
   OperandMatchResultTy parseInterpAttr(OperandVector &Operands);
   OperandMatchResultTy parseSOppBrTarget(OperandVector &Operands);
 
   bool parseSwizzleOperands(const unsigned OpNum, int64_t* Op,
if (trySkipId("offset")) {
     }
 
     Operands.push_back(AMDGPUOperand::CreateImm(this, Imm, S, AMDGPUOperand::ImmTySwizzle));
 
     return Ok? MatchOperand_Success : MatchOperand_ParseFail;
   } else {
     // Swizzle "offset" operand is optional.
     // If it is omitted, try parsing other optional operands.
-    return parseOptionalOperand(Operands);
+    return parseOptionalOpr(Operands);
   }
 }
 
 bool
 AMDGPUOperand::isSwizzle() const {
   return isImmTy(ImmTySwizzle);
 }
 
static const OptionalOperand AMDGPUOptionalOperandTable[] = {
   {"vm", AMDGPUOperand::ImmTyExpVM, true, nullptr},
   {"op_sel", AMDGPUOperand::ImmTyOpSel, false, nullptr},
   {"op_sel_hi", AMDGPUOperand::ImmTyOpSelHi, false, nullptr},
   {"neg_lo", AMDGPUOperand::ImmTyNegLo, false, nullptr},
   {"neg_hi", AMDGPUOperand::ImmTyNegHi, false, nullptr}
 };
 
 OperandMatchResultTy AMDGPUAsmParser::parseOptionalOperand(OperandVector &Operands) {
+  unsigned size = Operands.size();
+  assert(size > 0);
+
+  OperandMatchResultTy res = parseOptionalOpr(Operands);
+
+  // This is a hack to enable hardcoded mandatory operands which follow
+  // optional operands.
+  //
+  // Current design assumes that all operands after the first optional operand
+  // are also optional. However implementation of some instructions violates
+  // this rule (see e.g. flat/global atomic which have hardcoded 'glc' operands).
+  //
+  // To alleviate this problem, we have to (implicitly) parse extra operands
+  // to make sure autogenerated parser of custom operands never hit hardcoded
+  // mandatory operands.
+
+  if (size == 1 || ((AMDGPUOperand &)*Operands[size - 1]).isRegKind()) {
+
+    // We have parsed the first optional operand.
+    // Parse as many operands as necessary to skip all mandatory operands.
+
+    for (unsigned i = 0; i < MAX_OPR_LOOKAHEAD; ++i) {
+      if (res != MatchOperand_Success ||
+          getLexer().is(AsmToken::EndOfStatement)) break;
+      if (getLexer().is(AsmToken::Comma)) Parser.Lex();
+      res = parseOptionalOpr(Operands);
+    }
+  }
+
+  return res;
+}
+
+OperandMatchResultTy AMDGPUAsmParser::parseOptionalOpr(OperandVector &Operands) {
   OperandMatchResultTy res;
   for (const OptionalOperand &Op : AMDGPUOptionalOperandTable) {
     // try to parse any optional operand here
     if (Op.IsBit) {
       res = parseNamedBit(Op.Name, Operands, Op.Type);
     } else if (Op.Type == AMDGPUOperand::ImmTyOModSI) {
       res = parseOModOperand(Operands);
     } else if (Op.Type == AMDGPUOperand::ImmTySdwaDstSel ||

test/MC/AMDGPU/flat-gfx9.s

 flat_atomic_add v[3:4], v5 offset:8 slc
 // GFX9: flat_atomic_add v[3:4], v5 offset:8 slc ; encoding: [0x08,0x00,0x0a,0xdd,0x03,0x05,0x00,0x00]
 // VIERR: :1: error: invalid operand for instruction
 
 flat_atomic_add v[3:4], v5 inst_offset:8 slc
 // GFX9: flat_atomic_add v[3:4], v5 offset:8 slc ; encoding: [0x08,0x00,0x0a,0xdd,0x03,0x05,0x00,0x00]
 // VIERR: :1: error: invalid operand for instruction
 
+flat_atomic_cmpswap v[1:2], v[3:4] offset:4095
+// GFX9: flat_atomic_cmpswap v[1:2], v[3:4] offset:4095 ; encoding: [0xff,0x0f,0x04,0xdd,0x01,0x03,0x00,0x00]
+// VIERR: :1: error: invalid operand for instruction
+
+flat_atomic_cmpswap v[1:2], v[3:4] offset:4095 slc
+// GFX9: flat_atomic_cmpswap v[1:2], v[3:4] offset:4095 slc ; encoding: [0xff,0x0f,0x06,0xdd,0x01,0x03,0x00,0x00]
+// VIERR: :1: error: invalid operand for instruction
+
+flat_atomic_cmpswap v[1:2], v[3:4]
+// GFX9: flat_atomic_cmpswap v[1:2], v[3:4] ; encoding: [0x00,0x00,0x04,0xdd,0x01,0x03,0x00,0x00]
+// VI:   flat_atomic_cmpswap v[1:2], v[3:4] ; encoding: [0x00,0x00,0x04,0xdd,0x01,0x03,0x00,0x00]
+
+flat_atomic_cmpswap v[1:2], v[3:4] slc
+// GFX9: flat_atomic_cmpswap v[1:2], v[3:4] slc ; encoding: [0x00,0x00,0x06,0xdd,0x01,0x03,0x00,0x00]
+// VI:   flat_atomic_cmpswap v[1:2], v[3:4] slc ; encoding: [0x00,0x00,0x06,0xdd,0x01,0x03,0x00,0x00]
+
+flat_atomic_cmpswap v[1:2], v[3:4] offset:4095 glc
+// GCNERR: error: invalid operand for instruction
+
+flat_atomic_cmpswap v[1:2], v[3:4] glc
+// GCNERR: error: invalid operand for instruction
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4] offset:4095 glc
+// GFX9: flat_atomic_cmpswap v0, v[1:2], v[3:4] offset:4095 glc ; encoding: [0xff,0x0f,0x05,0xdd,0x01,0x03,0x00,0x00]
+// VIERR: :1: error: invalid operand for instruction
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4] offset:4095 glc slc
+// GFX9: flat_atomic_cmpswap v0, v[1:2], v[3:4] offset:4095 glc slc ; encoding: [0xff,0x0f,0x07,0xdd,0x01,0x03,0x00,0x00]
+// VIERR: :1: error: invalid operand for instruction
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4] glc
+// GFX9: flat_atomic_cmpswap v0, v[1:2], v[3:4] glc ; encoding: [0x00,0x00,0x05,0xdd,0x01,0x03,0x00,0x00]
+// VI:   flat_atomic_cmpswap v0, v[1:2], v[3:4] glc ; encoding: [0x00,0x00,0x05,0xdd,0x01,0x03,0x00,0x00]
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4] glc slc
+// GFX9: flat_atomic_cmpswap v0, v[1:2], v[3:4] glc slc ; encoding: [0x00,0x00,0x07,0xdd,0x01,0x03,0x00,0x00]
+// VI:   flat_atomic_cmpswap v0, v[1:2], v[3:4] glc slc ; encoding: [0x00,0x00,0x07,0xdd,0x01,0x03,0x00,0x00]
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4]
+// GFX9: flat_atomic_cmpswap v0, v[1:2], v[3:4] glc ; encoding: [0x00,0x00,0x05,0xdd,0x01,0x03,0x00,0x00]
+// VI:   flat_atomic_cmpswap v0, v[1:2], v[3:4] glc ; encoding: [0x00,0x00,0x05,0xdd,0x01,0x03,0x00,0x00]
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4] offset:4095
+// GCNERR: error: too few operands for instruction
+
+flat_atomic_cmpswap v0, v[1:2], v[3:4] slc
+// GCNERR: error: invalid operand for instruction
+
 flat_atomic_swap v[3:4], v5 offset:16
 // GFX9: flat_atomic_swap v[3:4], v5 offset:16 ; encoding: [0x10,0x00,0x00,0xdd,0x03,0x05,0x00,0x00]
 // VIERR: :1: error: invalid operand for instruction
 
 flat_store_dword v[3:4], v1 offset:16
 // GFX9: flat_store_dword v[3:4], v1 offset:16 ; encoding: [0x10,0x00,0x70,0xdc,0x03,0x01,0x00,0x00]
 // VIERR: :1: error: invalid operand for instruction
 

test/MC/Disassembler/AMDGPU/flat_gfx9.txt

 # RUN: llvm-mc -arch=amdgcn -mcpu=gfx900 -disassemble -show-encoding < %s | FileCheck %s
 
 # CHECK: flat_atomic_add v[0:1], v0    ; encoding: [0x00,0x00,0x08,0xdd,0x00,0x00,0x00,0x00]
 0x00,0x00,0x08,0xdd,0x00,0x00,0x00,0x00
 
 # CHECK: flat_atomic_add v[0:1], v0 offset:7    ; encoding: [0x07,0x00,0x08,0xdd,0x00,0x00,0x00,0x00]
 0x07,0x00,0x08,0xdd,0x00,0x00,0x00,0x00
 
 # CHECK: flat_atomic_add v0, v[0:1], v0 offset:4095 glc    ; encoding: [0xff,0x0f,0x09,0xdd,0x00,0x00,0x00,0x00]
 0xff,0x0f,0x09,0xdd,0x00,0x00,0x00,0x00
 
+# CHECK: flat_atomic_add v0, v[0:1], v0 offset:4095 glc slc ; encoding: [0xff,0x0f,0x0b,0xdd,0x00,0x00,0x00,0x00]
+0xff,0x0f,0x0b,0xdd,0x00,0x00,0x00,0x00
+
+# CHECK: flat_atomic_add v0, v[0:1], v0 glc ; encoding: [0x00,0x00,0x09,0xdd,0x00,0x00,0x00,0x00]
+0x00,0x00,0x09,0xdd,0x00,0x00,0x00,0x00
+
+# CHECK: flat_atomic_add v0, v[0:1], v0 glc slc ; encoding: [0x00,0x00,0x0b,0xdd,0x00,0x00,0x00,0x00]
+0x00,0x00,0x0b,0xdd,0x00,0x00,0x00,0x00
+
+# CHECK: flat_atomic_add v[0:1], v0 slc  ; encoding: [0x00,0x00,0x0a,0xdd,0x00,0x00,0x00,0x00]
+0x00,0x00,0x0a,0xdd,0x00,0x00,0x00,0x00
+
 # CHECK: flat_atomic_add v[0:1], v0 offset:4095 slc    ; encoding: [0xff,0x0f,0x0a,0xdd,0x00,0x00,0x00,0x00]
 0xff,0x0f,0x0a,0xdd,0x00,0x00,0x00,0x00
 
 # CHECK: flat_load_dword v0, v[0:1]    ; encoding: [0x00,0x00,0x50,0xdc,0x00,0x00,0x00,0x00]
 0x00,0x00,0x50,0xdc,0x00,0x00,0x00,0x00
 
 # CHECK: flat_load_dword v0, v[0:1] offset:7    ; encoding: [0x07,0x00,0x50,0xdc,0x00,0x00,0x00,0x00]
 0x07,0x00,0x50,0xdc,0x00,0x00,0x00,0x00