[AMDGPU][MC] Verify selection of LDS MUBUF opcodes

[dp]

Summary

MUBUF lds opcodes may be selected even if lds modifier is not specified.
See bug 56755.

[foad]

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

4495–4498

What do these lines achieve?

[dp]

Added a comment.

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

4495–4498

These lines exclude checks for cases when there are separate DMA opcodes. In these cases, incorrect opcode selection is not possible.

GFX940 FLAT opcodes do not support lds modifier, but there are separate DMA opcodes global_load_lds_dword, etc.

Likewise, GFX11 has buffer_load_lds_b32, etc; MUBUF lds modifier is not supported.

[foad]

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

4495–4498

OK. I thought those cases would be handled correctly anyway, because we would return true on line 4505, but maybe I am wrong.

[dp]

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

4495–4498

You are right, the check for MUBUF is optional, but GFX940 FLAT LDS opcodes have VALU=1.

[foad]

LGTM (with or without any changes in validateLdsDMA).

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

private:
   bool validateMFMA(const MCInst &Inst, const OperandVector &Operands);
   bool validateAGPRLdSt(const MCInst &Inst) const;
   bool validateVGPRAlign(const MCInst &Inst) const;
   bool validateBLGP(const MCInst &Inst, const OperandVector &Operands);
   bool validateGWS(const MCInst &Inst, const OperandVector &Operands);
   bool validateDivScale(const MCInst &Inst);
   bool validateCoherencyBits(const MCInst &Inst, const OperandVector &Operands,
                              const SMLoc &IDLoc);
-  bool validateFlatLdsDMA(const MCInst &Inst, const OperandVector &Operands,
-                          const SMLoc &IDLoc);
+  bool validateLdsDMA(uint64_t Enc, const MCInst &Inst,
+                      const OperandVector &Operands, const SMLoc &IDLoc);
   bool validateExeczVcczOperands(const OperandVector &Operands);
   Optional<StringRef> validateLdsDirect(const MCInst &Inst);
   unsigned getConstantBusLimit(unsigned Opcode) const;
   bool usesConstantBus(const MCInst &Inst, unsigned OpIdx);
   bool isInlineConstant(const MCInst &Inst, unsigned OpIdx) const;
   unsigned findImplicitSGPRReadInVOP(const MCInst &Inst) const;
 
   bool isSupportedMnemo(StringRef Mnemo,
if (CPol & CPol::GLC) {
                           : "instruction must not use glc");
       return false;
     }
   }
 
   return true;
 }
 
-bool AMDGPUAsmParser::validateFlatLdsDMA(const MCInst &Inst,
-                                         const OperandVector &Operands,
-                                         const SMLoc &IDLoc) {
-  if (isGFX940())
+bool AMDGPUAsmParser::validateLdsDMA(uint64_t Enc, const MCInst &Inst,
+  assert(Enc == SIInstrFlags::FLAT || Enc == SIInstrFlags::MUBUF);
+
+  // Exclude cases when there are separate DMA opcodes.
+  // In these cases, incorrect opcode selection is not possible.
+  if (Enc == SIInstrFlags::FLAT && isGFX940())
+    return true;
+  if (Enc == SIInstrFlags::MUBUF && isGFX11Plus())
     return true;
 
   uint64_t TSFlags = MII.get(Inst.getOpcode()).TSFlags;
-  if ((TSFlags & (SIInstrFlags::VALU | SIInstrFlags::FLAT)) !=
-      (SIInstrFlags::VALU | SIInstrFlags::FLAT))
+  if ((TSFlags & (SIInstrFlags::VALU | Enc)) !=
+      (SIInstrFlags::VALU | Enc))
     return true;
-  // This is FLAT LDS DMA.
+  // This is FLAT/MUBUF LDS DMA.
 
   SMLoc S = getImmLoc(AMDGPUOperand::ImmTyLDS, Operands);
   StringRef CStr(S.getPointer());
   if (!CStr.startswith("lds")) {
-    // This is incorrectly selected LDS DMA version of a FLAT load opcode.
-    // And LDS version should have 'lds' modifier, but it follows optional
-    // operands so its absense is ignored by the matcher.
-    Error(IDLoc, "invalid operands for instruction");
+    // This is incorrectly selected LDS DMA version of a FLAT/MUBUF load
+    // opcode. And LDS version should have 'lds' modifier, but it follows
+    // optional operands so its absense is ignored by the matcher.
+    Error(IDLoc, "missing dst operand or lds modifier");
     return false;
   }
 
   return true;
 }
 
 bool AMDGPUAsmParser::validateExeczVcczOperands(const OperandVector &Operands) {
   if (!isGFX11Plus())
bool AMDGPUAsmParser::validateInstruction(const MCInst &Inst,
   if (!validateDivScale(Inst)) {
     Error(IDLoc, "ABS not allowed in VOP3B instructions");
     return false;
   }
   if (!validateExeczVcczOperands(Operands)) {
     return false;
   }
 
-  if (!validateFlatLdsDMA(Inst, Operands, IDLoc)) {
+  if (!validateLdsDMA(SIInstrFlags::FLAT, Inst, Operands, IDLoc)) {
+    return false;
+  }
+
+  if (!validateLdsDMA(SIInstrFlags::MUBUF, Inst, Operands, IDLoc)) {
     return false;
   }
 
   return true;
 }
 
 static std::string AMDGPUMnemonicSpellCheck(StringRef S,
                                             const FeatureBitset &FBS,

llvm/test/MC/AMDGPU/gfx10_err_pos.s

 // message operation does not support streams
 
 s_sendmsg sendmsg(MSG_GS_DONE, GS_OP_NOP, 0)
 // CHECK: error: message operation does not support streams
 // CHECK-NEXT:{{^}}s_sendmsg sendmsg(MSG_GS_DONE, GS_OP_NOP, 0)
 // CHECK-NEXT:{{^}}                                          ^
 
 //==============================================================================
+// missing dst operand or lds modifier
+
+buffer_load_dword off, s[8:11], s3
+// CHECK: error: missing dst operand or lds modifier
+// CHECK-NEXT:{{^}}buffer_load_dword off, s[8:11], s3
+// CHECK-NEXT:{{^}}^
+
+buffer_load_dword off, s[8:11], s3 offset:1
+// CHECK: error: missing dst operand or lds modifier
+// CHECK-NEXT:{{^}}buffer_load_dword off, s[8:11], s3 offset:1
+// CHECK-NEXT:{{^}}^
+
+//==============================================================================
 // missing message operation
 
 s_sendmsg sendmsg(MSG_SYSMSG)
 // CHECK: error: missing message operation
 // CHECK-NEXT:{{^}}s_sendmsg sendmsg(MSG_SYSMSG)
 // CHECK-NEXT:{{^}}                  ^
 
 //==============================================================================

llvm/test/MC/AMDGPU/gfx9-asm-err.s

 
 v_cvt_u32_f64 v5, v[0:1] quad_perm:[0,2,1,1] row_mask:0xf bank_mask:0xf
 // GFX9ERR: error: invalid operand for instruction
 
 global_load_lds_dword v[2:3], off
 // GFX9ERR: error: instruction not supported on this GPU
 
 global_load_dword v[2:3], off
-// GFX9ERR: error: invalid operands for instruction
+// GFX9ERR: error: missing dst operand or lds modifier
 
 scratch_load_dword v2, off, offset:256
-// GFX9ERR: error: invalid operands for instruction
+// GFX9ERR: error: missing dst operand or lds modifier

llvm/test/MC/AMDGPU/gfx9_err_pos.s

 // CHECK-NEXT:{{^}}                  ^
 
 v_add_i16 v5, 0.5, v2
 // CHECK: error: literal operands are not supported
 // CHECK-NEXT:{{^}}v_add_i16 v5, 0.5, v2
 // CHECK-NEXT:{{^}}              ^
 
 //==============================================================================
+// missing dst operand or lds modifier
+
+buffer_load_dword off, s[8:11], s3
+// CHECK: error: missing dst operand or lds modifier
+// CHECK-NEXT:{{^}}buffer_load_dword off, s[8:11], s3
+// CHECK-NEXT:{{^}}^
+
+buffer_load_dword off, s[8:11], s3 offset:1
+// CHECK: error: missing dst operand or lds modifier
+// CHECK-NEXT:{{^}}buffer_load_dword off, s[8:11], s3 offset:1
+// CHECK-NEXT:{{^}}^
+
+//==============================================================================
 // r128 modifier is not supported on this GPU
 
 image_atomic_add v10, v6, s[8:15] dmask:0x1 r128
 // CHECK: error: r128 modifier is not supported on this GPU
 // CHECK-NEXT:{{^}}image_atomic_add v10, v6, s[8:15] dmask:0x1 r128
 // CHECK-NEXT:{{^}}                                            ^
 
 image_atomic_add v10, v6, s[8:15] dmask:0x1 nor128

llvm/test/MC/AMDGPU/mubuf.s

 
 buffer_store_lds_dword s[4:7], s8 offset:4 lds tfe
 // NOSICI: error: instruction not supported on this GPU
 // NOVI:   error: invalid operand for instruction
 
 buffer_store_lds_dword s[4:7], s8 offset:4 tfe lds
 // NOSICI: error: instruction not supported on this GPU
 // NOVI:   error: invalid operand for instruction
+
+buffer_load_dword off, s[8:11], s3
+// NOSICIVI: error: missing dst operand or lds modifier
+
+buffer_load_dword off, s[8:11], s3 offset:1
+// NOSICIVI: error: missing dst operand or lds modifier