[AMDGPU][MC] Corrected handling of tied src for atomic return MUBUF opcodes

[dp]

Summary

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

[arsenm]

Why is an inline immediate allowed here? I thought these could only be an SGPR or 0

[dp]

Shader programming documents state that MUBUF SOFFSET is "SGPR to supply unsigned byte offset. Must be an SGPR, M0 or inline constant."
Also note that our tests are based on SP3 tables which also allow inline constants.
Of course, using fp inline constants in this context is meaningless.

[artem.tamazov]

In D59305#1428852, @dp wrote:

Shader programming documents state that MUBUF SOFFSET is "SGPR to supply unsigned byte offset. Must be an SGPR, M0 or inline constant."
Also note that our tests are based on SP3 tables which also allow inline constants.
Of course, using fp inline constants in this context is meaningless.

However, assembler can correctly fold the 0x3f000000 constant value into f32 inline constant shown as 0.5 during disassembly.

[dp]

Could you explain what issues you see in this change? I'm trying to make assembler as close to SP3 as possible.

[dp]

ping

llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
                                const OperandVector &Operands,
                                bool IsAtomic,
                                bool IsAtomicReturn,
                                bool IsLds) {
   bool IsLdsOpcode = IsLds;
   bool HasLdsModifier = false;
   OptionalImmIndexMap OptionalIdx;
   assert(IsAtomicReturn ? IsAtomic : true);
+  unsigned FirstOperandIdx = 1;
 
-  for (unsigned i = 1, e = Operands.size(); i != e; ++i) {
+  for (unsigned i = FirstOperandIdx, e = Operands.size(); i != e; ++i) {
     AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);
 
     // Add the register arguments
     if (Op.isReg()) {
       Op.addRegOperands(Inst, 1);
+      // Insert a tied src for atomic return dst.
+      // This cannot be postponed as subsequent calls to
+      // addImmOperands rely on correct number of MC operands.
+      if (IsAtomicReturn && i == FirstOperandIdx)
+        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;
     }
void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
   if (IsLdsOpcode && !HasLdsModifier) {
     int NoLdsOpcode = AMDGPU::getMUBUFNoLdsInst(Inst.getOpcode());
     if (NoLdsOpcode != -1) { // Got lds version - correct it.
       Inst.setOpcode(NoLdsOpcode);
       IsLdsOpcode = false;
     }
   }
 
-  // Copy $vdata_in operand and insert as $vdata for MUBUF_Atomic RTN insns.
-  if (IsAtomicReturn) {
-    MCInst::iterator I = Inst.begin(); // $vdata_in is always at the beginning.
-    Inst.insert(I, *I);
-  }
-
   addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyOffset);
   if (!IsAtomic) { // glc is hard-coded.
     addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyGLC);
   }
   addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTySLC);
 
   if (!IsLdsOpcode) { // tfe is not legal with lds opcodes
     addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyTFE);

llvm/trunk/test/MC/AMDGPU/mubuf.s

 buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc
 // SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc ; encoding: [0x04,0x70,0xf0,0xe0,0x02,0x01,0x02,0xb8]
 // VI:   buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc ; encoding: [0x04,0x70,0x2c,0xe1,0x02,0x01,0x02,0xb8]
 
 buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc slc
 // SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc slc ; encoding: [0x04,0x70,0xf0,0xe0,0x02,0x01,0x42,0xb8]
 // VI:   buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc slc ; encoding: [0x04,0x70,0x2e,0xe1,0x02,0x01,0x02,0xb8]
 
+buffer_atomic_add v5, off, s[8:11], 0.5 offset:4095 glc
+// SICI: buffer_atomic_add v5, off, s[8:11], 0.5 offset:4095 glc ; encoding: [0xff,0x4f,0xc8,0xe0,0x00,0x05,0x02,0xf0]
+// VI:   buffer_atomic_add v5, off, s[8:11], 0.5 offset:4095 glc ; encoding: [0xff,0x4f,0x08,0xe1,0x00,0x05,0x02,0xf0]
+
+buffer_atomic_add v5, off, s[8:11], 0.15915494 offset:4095 glc
+// NOSICI: error: invalid operand for instruction
+// VI:   buffer_atomic_add v5, off, s[8:11], 0.15915494 offset:4095 glc ; encoding: [0xff,0x4f,0x08,0xe1,0x00,0x05,0x02,0xf8]
+
 //===----------------------------------------------------------------------===//
 // Lds support
 //===----------------------------------------------------------------------===//
 
 buffer_load_sbyte v5, off, s[8:11], s3 lds
 // SICI: buffer_load_sbyte v5, off, s[8:11], s3 lds ; encoding: [0x00,0x00,0x25,0xe0,0x00,0x05,0x02,0x03]
 // VI:   buffer_load_sbyte v5, off, s[8:11], s3 lds ; encoding: [0x00,0x00,0x45,0xe0,0x00,0x05,0x02,0x03]