[AMDGPU][llvm-mc] Fixes to support buffer atomics.

[artem.tamazov]

Summary

Fixes for MUBUF_Atomic instructions to make operand list valid:

• For RTN insns, make a copy of $vdata_in operand as $vdata.
• Do not add operand for GLC, it is hardcoded and comes as a token.

Workaround to avoid adding multiple optional default operands; more info in comments.
Tests added.

[artem.tamazov]

Ping. BTW I am going to add some tests for buffer atomics.

[artem.tamazov]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

1485–1487

Previously added operand shall be examined here. Going to fix and update the patch soon.

[artem.tamazov]

Don't add operand for GLC (add new cvtMubufAtomic() for that).
Fix w/a in parseIntWithPrefix().
Add ~1/3 of planned tests (more tomorrow).

[artem.tamazov]

Tests added, all done.

[SamWot]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

90–100

I think there is no need in this define. This workaround is always enabled and I don't see when it could be disabled. When we will be able to get rid of this we will just remove this type of operands.

670–675

Move this to private section of AMDGPUAsmParser where all private function reside. This really harms readability of code.

2142

Do we need this check at all?
At least you can replace it with simple assert

[SamWot]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

2141

You can create converter just for atomics with return value instead of converter for all atomics. In that case there would be no need in this check. This would be more stable because in change I'm currently working glc would be always parsed as immediate (not token) and this check will fail.

[artem.tamazov]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

90–100

While there is no programmatic purpose of this macro (in this specific case), there is another duties for it.
It replaces comments like "// workaround for..." which I would insert otherwise.
It clearly denotes lines of code, so it is extremely easy to remove the workaround.
It provides programmatic link to the textual description of the workaround.
Will remove, if you insist ))

[artem.tamazov]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

2123

Good idea, thanks!

[SamWot]

LGTM

[artem.tamazov]

I am going to commit this in 24hrs if no objections/comments arrive.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

 
 using namespace llvm;
 
+// In some cases (e.g. buffer atomic instructions) MatchOperandParserImpl()
+// may invoke tryCustomParseOperand() multiple times with the same MCK value.
+// That leads to adding of the same "default" operand multiple times in a row,
+// which is wrong. The workaround adds only the 1st default operand, while for
+// the rest the "dummy" operands being added. The reason for dummies is that if
+// we just skip adding an operand, then parser would get stuck in endless loop.
+// Dummies shall be removed prior matching & emitting MCInsts.
+//
+// Comment out this macro to disable the workaround.
+#define WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+
 namespace {
 
 struct OptionalOperand;

     Immediate,
     Register,
     Expression
+#ifdef WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+    ,Dummy
+#endif
   } Kind;
 
   SMLoc StartLoc, EndLoc;

     }
   }
 
+#ifdef WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+  bool isDummy() const {
+    return Kind == Dummy;
+  }
+#endif
+
   bool isToken() const override {
     return Kind == Token;
   }

     case Expression:
       OS << "<expr " << *Expr << '>';
       break;
+#ifdef WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+    case Dummy:
+      OS << "<dummy>";
+      break;
+#endif
     }
   }
 

     return Op;
   }
 
+#ifdef WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+  static AMDGPUOperand::Ptr CreateDummy(SMLoc S) {
+    auto Op = llvm::make_unique<AMDGPUOperand>(Dummy);
+    Op->StartLoc = S;
+    Op->EndLoc = S;
+    return Op;
+  }
+#endif
+
   bool isSWaitCnt() const;
   bool isHwreg() const;
   bool isSendMsg() const;

   OperandMatchResultTy parseSOppBrTarget(OperandVector &Operands);
   AMDGPUOperand::Ptr defaultHwreg() const;
 
-
-  void cvtMubuf(MCInst &Inst, const OperandVector &Operands);
+private:
+  void cvtMubufImpl(MCInst &Inst, const OperandVector &Operands, bool IsAtomic);
+public:
+  void cvtMubuf(MCInst &Inst, const OperandVector &Operands) { cvtMubufImpl(Inst, Operands, false); }
+  void cvtMubufAtomic(MCInst &Inst, const OperandVector &Operands) { cvtMubufImpl(Inst, Operands, true); }
   AMDGPUOperand::Ptr defaultMubufOffset() const;
   AMDGPUOperand::Ptr defaultGLC() const;
   AMDGPUOperand::Ptr defaultSLC() const;

                                               bool MatchingInlineAsm) {
   MCInst Inst;
 
+#ifdef WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+  // Remove dummies prior matching. Iterate backwards becase vector::erase()
+  // invalidates all iterators which refer after erase point.
+  for (auto I = Operands.rbegin(), E = Operands.rend(); I != E; ) {
+    auto X = I++;
+    if (static_cast<AMDGPUOperand*>(X->get())->isDummy()) {
+      Operands.erase(X.base() -1);
+    }
+  }
+#endif
+
   switch (MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm)) {
     default: break;
     case Match_Success:

   }
 
   Operands.push_back(AMDGPUOperand::CreateImm(Value, S, ImmTy));
+
+#ifdef WORKAROUND_USE_DUMMY_OPERANDS_INSTEAD_MUTIPLE_DEFAULT_OPERANDS
+  if (Value == Default && AddDefault) {
+    // Reverse lookup in previously added operands (skip just added one)
+    // for the first non-dummy operand. If it is of the same type,
+    // then replace just added default operand with dummy.
+    for (auto I = Operands.rbegin(), E = Operands.rend(); I != E; ++I) {
+      if (I == Operands.rbegin())
+        continue;
+      if (static_cast<AMDGPUOperand*>(I->get())->isDummy())
+        continue;
+      if (static_cast<AMDGPUOperand*>(I->get())->isImmTy(ImmTy)) {
+        Operands.pop_back();
+        Operands.push_back(AMDGPUOperand::CreateDummy(S)); // invalidates iterators
+        break;
+      }
+    }
+  }
+#endif
   return MatchOperand_Success;
 }
 

   return AMDGPUOperand::CreateImm(0, SMLoc(), AMDGPUOperand::ImmTyTFE);
 }
 
-void AMDGPUAsmParser::cvtMubuf(MCInst &Inst,
-                               const OperandVector &Operands) {
+void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
+                               const OperandVector &Operands,
+                               bool IsAtomic) {
   OptionalImmIndexMap OptionalIdx;
+  bool IsActomicRtn = false;
 
   for (unsigned i = 1, e = Operands.size(); i != e; ++i) {
     AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);

     // Handle tokens like 'offen' which are sometimes hard-coded into the
     // asm string.  There are no MCInst operands for these.
     if (Op.isToken()) {
+      if (Op.getToken() == "glc") { // Detect MUBUF_Atomic RTN insns.
+        if (IsAtomic) {
+          IsActomicRtn = true;
+        } else {
+          llvm_unreachable("Only buffer_atomic instructions may have hard-coded 'glc'.");
+        }
+      }
       continue;
     }
     assert(Op.isImm());

     // Handle optional arguments
     OptionalIdx[Op.getImmTy()] = i;
   }
+  
+  // Copy $vdata_in operand and insert as $vdata for MUBUF_Atomic RTN insns.
+  if (IsActomicRtn) {
+    MCInst::iterator I = Inst.begin(); // $vdata_in is always at the beginning.
+    Inst.insert(I, *I);
+  }
 
   addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyOffset);
-  addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyGLC);
+  if (!IsAtomic) { // glc is hard-coded in atomics.
+    addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyGLC);
+  }
   addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTySLC);
   addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyTFE);
 }

lib/Target/AMDGPU/SIInstrInfo.td

   let mayStore = 1, mayLoad = 1, hasPostISelHook = 1, hasSideEffects = 1 in {
 
     // No return variants
-    let glc = 0 in {
+    let glc = 0, AsmMatchConverter = "cvtMubufAtomic" in {
 
       defm _ADDR64 : MUBUFAtomicAddr64_m <
         op, name#"_addr64", (outs),

 
     // Variant that return values
     let glc = 1, Constraints = "$vdata = $vdata_in",
+        AsmMatchConverter = "cvtMubufAtomic",
         DisableEncoding = "$vdata_in"  in {
 
       defm _RTN_ADDR64 : MUBUFAtomicAddr64_m <
         op, name#"_rtn_addr64", (outs rc:$vdata),
         (ins rc:$vdata_in, VReg_64:$vaddr, SReg_128:$srsrc,
              SCSrc_32:$soffset, offset:$offset, slc:$slc),
-        name#" $vdata, $vaddr, $srsrc, $soffset addr64"#"$offset"#" glc"#"$slc",
+        name#" $vdata, $vaddr, $srsrc, $soffset addr64$offset glc$slc",
         [(set vt:$vdata,
          (atomic (MUBUFAddr64Atomic v4i32:$srsrc, i64:$vaddr, i32:$soffset,
                               i16:$offset, i1:$slc), vt:$vdata_in))], 1

         op, name#"_rtn_offset", (outs rc:$vdata),
         (ins rc:$vdata_in, SReg_128:$srsrc, SCSrc_32:$soffset,
              offset:$offset, slc:$slc),
-        name#" $vdata, off, $srsrc, $soffset $offset glc$slc",
+        name#" $vdata, off, $srsrc, $soffset$offset glc$slc",
         [(set vt:$vdata,
          (atomic (MUBUFOffsetAtomic v4i32:$srsrc, i32:$soffset, i16:$offset,
                                     i1:$slc), vt:$vdata_in))], 1

           op, name#"_rtn_offen", (outs rc:$vdata),
           (ins rc:$vdata_in, VGPR_32:$vaddr, SReg_128:$srsrc, SCSrc_32:$soffset,
                 offset:$offset, slc:$slc),
-          name#" $vdata, $vaddr, $srsrc, $soffset offen"#"$offset"#" glc"#"$slc",
+          name#" $vdata, $vaddr, $srsrc, $soffset offen$offset glc$slc",
           [], 1
         >;
       }

           op, name#"_rtn_idxen", (outs rc:$vdata),
           (ins rc:$vdata_in, VGPR_32:$vaddr, SReg_128:$srsrc, SCSrc_32:$soffset,
                 offset:$offset, slc:$slc),
-          name#" $vdata, $vaddr, $srsrc, $soffset idxen"#"$offset"#" glc"#"$slc",
+          name#" $vdata, $vaddr, $srsrc, $soffset idxen$offset glc$slc",
           [], 1
         >;
       }

           op, name#"_rtn_bothen", (outs rc:$vdata),
           (ins rc:$vdata_in, VReg_64:$vaddr, SReg_128:$srsrc, SCSrc_32:$soffset,
                 offset:$offset, slc:$slc),
-          name#" $vdata, $vaddr, $srsrc, $soffset idxen offen"#"$offset"#" glc"#"$slc",
+          name#" $vdata, $vaddr, $srsrc, $soffset idxen offen$offset glc$slc",
           [], 1
         >;
       }

test/MC/AMDGPU/mubuf.s

 // VI: buffer_wbinvl1_vol ; encoding: [0x00,0x00,0xfc,0xe0,0x00,0x00,0x00,0x00]
 // NOSI: error: instruction not supported on this GPU
 
-// TODO: Atomics
+//===----------------------------------------------------------------------===//
+// Atomics
+//===----------------------------------------------------------------------===//
+buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 glc
+// SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 glc ; encoding: [0x00,0xc0,0xf0,0xe0,0x02,0x01,0x02,0xb8]
+// NOVI: error: instruction not supported on this GPU
+
+buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 glc slc
+// SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 glc slc ; encoding: [0x00,0xc0,0xf0,0xe0,0x02,0x01,0x42,0xb8]
+// NOVI: error: instruction not supported on this GPU
+
+buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 offset:4 glc
+// SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 offset:4 glc ; encoding: [0x04,0xc0,0xf0,0xe0,0x02,0x01,0x02,0xb8]
+// NOVI: error: instruction not supported on this GPU
+
+buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 offset:4 glc slc
+// SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 addr64 offset:4 glc slc ; encoding: [0x04,0xc0,0xf0,0xe0,0x02,0x01,0x42,0xb8]
+// NOVI: error: instruction not supported on this GPU
+
+buffer_atomic_inc v1, off, s[8:11], 56 glc
+// SICI: buffer_atomic_inc v1, off, s[8:11], 56 glc ; encoding: [0x00,0x40,0xf0,0xe0,0x00,0x01,0x02,0xb8]
+// VI:   buffer_atomic_inc v1, off, s[8:11], 56 glc ; encoding: [0x00,0x40,0x2c,0xe1,0x00,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, off, s[8:11], 56 glc slc
+// SICI: buffer_atomic_inc v1, off, s[8:11], 56 glc slc ; encoding: [0x00,0x40,0xf0,0xe0,0x00,0x01,0x42,0xb8]
+// VI:   buffer_atomic_inc v1, off, s[8:11], 56 glc slc ; encoding: [0x00,0x40,0x2e,0xe1,0x00,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, off, s[8:11], 56 offset:4 glc
+// SICI: buffer_atomic_inc v1, off, s[8:11], 56 offset:4 glc ; encoding: [0x04,0x40,0xf0,0xe0,0x00,0x01,0x02,0xb8]
+// VI:   buffer_atomic_inc v1, off, s[8:11], 56 offset:4 glc ; encoding: [0x04,0x40,0x2c,0xe1,0x00,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, off, s[8:11], 56 offset:4 glc slc
+// SICI: buffer_atomic_inc v1, off, s[8:11], 56 offset:4 glc slc ; encoding: [0x04,0x40,0xf0,0xe0,0x00,0x01,0x42,0xb8]
+// VI:   buffer_atomic_inc v1, off, s[8:11], 56 offset:4 glc slc ; encoding: [0x04,0x40,0x2e,0xe1,0x00,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, v2, s[8:11], 56 offen glc
+// SICI: buffer_atomic_inc v1, v2, s[8:11], 56 offen glc ; encoding: [0x00,0x50,0xf0,0xe0,0x02,0x01,0x02,0xb8]
+// VI:   buffer_atomic_inc v1, v2, s[8:11], 56 offen glc ; encoding: [0x00,0x50,0x2c,0xe1,0x02,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, v2, s[8:11], 56 offen glc slc
+// SICI: buffer_atomic_inc v1, v2, s[8:11], 56 offen glc slc ; encoding: [0x00,0x50,0xf0,0xe0,0x02,0x01,0x42,0xb8]
+// VI:   buffer_atomic_inc v1, v2, s[8:11], 56 offen glc slc ; encoding: [0x00,0x50,0x2e,0xe1,0x02,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, v2, s[8:11], 56 offen offset:4 glc
+// SICI: buffer_atomic_inc v1, v2, s[8:11], 56 offen offset:4 glc ; encoding: [0x04,0x50,0xf0,0xe0,0x02,0x01,0x02,0xb8]
+// VI:   buffer_atomic_inc v1, v2, s[8:11], 56 offen offset:4 glc ; encoding: [0x04,0x50,0x2c,0xe1,0x02,0x01,0x02,0xb8]
+
+buffer_atomic_inc v1, v2, s[8:11], 56 offen offset:4 glc slc
+// SICI: buffer_atomic_inc v1, v2, s[8:11], 56 offen offset:4 glc slc ; encoding: [0x04,0x50,0xf0,0xe0,0x02,0x01,0x42,0xb8]
+// VI:   buffer_atomic_inc v1, v2, s[8:11], 56 offen offset:4 glc slc ; encoding: [0x04,0x50,0x2e,0xe1,0x02,0x01,0x02,0xb8]

test/MC/AMDGPU/trap.s

 v_readfirstlane_b32  ttmp8, v1
 // SICI: v_readfirstlane_b32 ttmp8, v1   ; encoding: [0x01,0x05,0xf0,0x7e]
 // VI:   v_readfirstlane_b32 ttmp8, v1   ; encoding: [0x01,0x05,0xf0,0x7e]
+
+buffer_atomic_inc  v1, off, ttmp[8:11], 56 glc
+// SICI: buffer_atomic_inc v1, off, ttmp[8:11], 56  glc ; encoding: [0x00,0x40,0xf0,0xe0,0x00,0x01,0x1e,0xb8]
+// VI:   buffer_atomic_inc v1, off, ttmp[8:11], 56  glc ; encoding: [0x00,0x40,0x2c,0xe1,0x00,0x01,0x1e,0xb8]