[AMDGPU][MC] Correct handling of mandatory literals

[dp]

Summary

Mandatory literals are incorrectly handled by the parser.

When the value of a mandatory literal is in the range of inline constants, it is not accounted for as a literal. As a result, assembler may accept instructions with several different literals. See this bug for more information.

[Joe_Nash]

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

3382

This looks a bit weird to me because isSISrcOperand used to be true for KImm, until I created the mandatory literal logic. I'm not sure what the desired semantics of isSISrcOperand are. Can you try making isSISrcOperand used to be true for KImm and seeing if there is any fallout?

[dp]

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

3382

isSISrcOperand does return true for KImm and seems it always did. I just added a limitation on KImm operands - they can no longer be interpreted as inline constants.

Before the mandatory literal logic was introduced, this limitation on KImm operands was unnecessary because only one literal was allowed for affected instructions.

Could you describe your concern in more detail?

[dp]

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

3382

Are you suggesting to see if it is feasible to make isSISrcOperand return false for KImm?

[Joe_Nash]

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

3382

Are you suggesting to see if it is feasible to make isSISrcOperand return false for KImm?

Yes.

Sorry, I said the reverse of what I meant.

[dp]

Rebase; update tests to add error line numbers.

[dp]

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

3382

I prepared a separate change on top of this patch to see how this can be done. Please take a look. I'm not sure if this would be that useful.

https://reviews.llvm.org/D139101

[Joe_Nash]

Ok, apart from that comment LGTM.

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

3382

Thanks for trying that. I agree it doesn't look too useful.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

1129–1130

Can you make this comment more precise?

[dp]

Update comment describing isSISrcOperand.

[cfang]

llvm/lib/Target/AMDGPU/SIDefines.h

207

These two definitions caused an issue. it makes OPERAND_INPUT_MODS = OPERAND_KIMM16 + 1, which is actually
OPERAND_REG_INLINE_AC_INT16 ( next enumerator). As a result, two unrelated enumerators have the same value.

I create a patch to address the issue -- we can put uninitialized enumerators together.
https://reviews.llvm.org/D141643

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

 // NB: This code is correct only when used to check constant
 // bus limitations because GFX7 support no f16 inline constants.
 // Note that there are no cases when a GFX7 opcode violates
 // constant bus limitations due to the use of an f16 constant.
 bool AMDGPUAsmParser::isInlineConstant(const MCInst &Inst,
                                        unsigned OpIdx) const {
   const MCInstrDesc &Desc = MII.get(Inst.getOpcode());
 
-  if (!AMDGPU::isSISrcOperand(Desc, OpIdx)) {
+  if (!AMDGPU::isSISrcOperand(Desc, OpIdx) ||
+      AMDGPU::isKImmOperand(Desc, OpIdx)) {
     return false;
   }
 
   const MCOperand &MO = Inst.getOperand(OpIdx);
 
   int64_t Val = MO.getImm();
   auto OpSize = AMDGPU::getOperandSize(Desc, OpIdx);
 

llvm/lib/Target/AMDGPU/SIDefines.h

enum OperandType : unsigned {
   OPERAND_REG_INLINE_C_LAST = OPERAND_REG_INLINE_AC_V2FP32,
 
   OPERAND_REG_INLINE_AC_FIRST = OPERAND_REG_INLINE_AC_INT16,
   OPERAND_REG_INLINE_AC_LAST = OPERAND_REG_INLINE_AC_V2FP32,
 
   OPERAND_SRC_FIRST = OPERAND_REG_IMM_INT32,
   OPERAND_SRC_LAST = OPERAND_REG_INLINE_C_LAST,
 
+  OPERAND_KIMM_FIRST = OPERAND_KIMM32,
+  OPERAND_KIMM_LAST = OPERAND_KIMM16,
+
   // Operand for source modifiers for VOP instructions
   OPERAND_INPUT_MODS,
 
   // Operand for SDWA instructions
   OPERAND_SDWA_VOPC_DST
 
 };
 }

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

 /// If \p Reg is a pseudo reg, return the correct hardware register given
 /// \p STI otherwise return \p Reg.
 unsigned getMCReg(unsigned Reg, const MCSubtargetInfo &STI);
 
 /// Convert hardware register \p Reg to a pseudo register
 LLVM_READNONE
 unsigned mc2PseudoReg(unsigned Reg);
 
-/// Can this operand also contain immediate values?
+/// Is this an AMDGPU specific source operand? These include registers,
+/// inline constants, literals and mandatory literals (KImm).
 bool isSISrcOperand(const MCInstrDesc &Desc, unsigned OpNo);
 
+/// Is this a KImm operand?
+bool isKImmOperand(const MCInstrDesc &Desc, unsigned OpNo);
+
 /// Is this floating-point operand?
 bool isSISrcFPOperand(const MCInstrDesc &Desc, unsigned OpNo);
 
 /// Does this operand support only inlinable literals?
 bool isSISrcInlinableOperand(const MCInstrDesc &Desc, unsigned OpNo);
 
 /// Get the size in bits of a register from the register class \p RC.
 unsigned getRegBitWidth(unsigned RCID);

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

 
 bool isSISrcOperand(const MCInstrDesc &Desc, unsigned OpNo) {
   assert(OpNo < Desc.NumOperands);
   unsigned OpType = Desc.OpInfo[OpNo].OperandType;
   return OpType >= AMDGPU::OPERAND_SRC_FIRST &&
          OpType <= AMDGPU::OPERAND_SRC_LAST;
 }
 
+bool isKImmOperand(const MCInstrDesc &Desc, unsigned OpNo) {
+  assert(OpNo < Desc.NumOperands);
+  unsigned OpType = Desc.OpInfo[OpNo].OperandType;
+  return OpType >= AMDGPU::OPERAND_KIMM_FIRST &&
+         OpType <= AMDGPU::OPERAND_KIMM_LAST;
+}
+
 bool isSISrcFPOperand(const MCInstrDesc &Desc, unsigned OpNo) {
   assert(OpNo < Desc.NumOperands);
   unsigned OpType = Desc.OpInfo[OpNo].OperandType;
   switch (OpType) {
   case AMDGPU::OPERAND_REG_IMM_FP32:
   case AMDGPU::OPERAND_REG_IMM_FP32_DEFERRED:
   case AMDGPU::OPERAND_REG_IMM_FP64:
   case AMDGPU::OPERAND_REG_IMM_FP16:

llvm/test/MC/AMDGPU/gfx10_asm_err.s

 v_fmaak_f32 v0, 0xff32, v0, 0x1122
 // GFX6-9: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // GFX10: :[[@LINE-2]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 
 v_fmamk_f32 v0, 0xff32, 0x1122, v0
 // GFX6-9: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // GFX10: :[[@LINE-2]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 
+v_fmaak_f32 v0, 0xff32, v0, 0
+// GFX6-9: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
+// GFX10: :[[@LINE-2]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
+v_fmaak_f16 v0, 0xff32, v0, 0
+// GFX6-9: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
+// GFX10: :[[@LINE-2]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
+v_fmamk_f32 v0, 0xff32, 1, v0
+// GFX6-9: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
+// GFX10: :[[@LINE-2]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
+v_fmamk_f16 v0, 0xff32, 1, v0
+// GFX6-9: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
+// GFX10: :[[@LINE-2]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
 //===----------------------------------------------------------------------===//
 // VOP2 E64.
 //===----------------------------------------------------------------------===//
 
 v_add_co_ci_u32 v5, 0, v1, v2, vcc
 // GFX6-7: :[[@LINE-1]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // GFX8-9: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // GFX10: :[[@LINE-3]]:{{[0-9]+}}: error: invalid operand for instruction

llvm/test/MC/AMDGPU/gfx11_asm_vop2_err.s

+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1100 %s 2>&1 | FileCheck --check-prefix=GFX11 --implicit-check-not=error: %s
+
+v_fmaak_f32 v0, 0xff32, v0, 0
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
+v_fmaak_f16 v0, 0xff32, v0, 0
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
+v_fmamk_f32 v0, 0xff32, 1, v0
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+
+v_fmamk_f16 v0, 0xff32, 1, v0
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed

llvm/test/MC/AMDGPU/gfx11_asm_vopd_err.s

 // GFX11-NEXT:{{^}}                                                                                   ^
 
 v_dual_fmamk_f32    v122, 0xdeadbeef, 0xdeadbeef, v161 ::  v_dual_fmamk_f32  v123, s0, 0x1234, v162
 // GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 // GFX11-NEXT:{{^}}v_dual_fmamk_f32    v122, 0xdeadbeef, 0xdeadbeef, v161 ::  v_dual_fmamk_f32  v123, s0, 0x1234, v162
 // GFX11-NEXT:{{^}}                          ^
 
 //===----------------------------------------------------------------------===//
+// Check that KImm operands are counted as literals
+// even if they look like inline constants.
+//===----------------------------------------------------------------------===//
+
+v_dual_fmamk_f32    v122, v74, 0, v161           ::  v_dual_lshlrev_b32  v247, 0xbabe, v99
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+// GFX11-NEXT:{{^}}v_dual_fmamk_f32    v122, v74, 0, v161           ::  v_dual_lshlrev_b32  v247, 0xbabe, v99
+// GFX11-NEXT:{{^}}                                                                               ^
+
+v_dual_add_f32      v5, 0xaf123456, v2           ::  v_dual_fmaak_f32     v6, v3, v1, 1.0
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+// GFX11-NEXT:{{^}}v_dual_add_f32      v5, 0xaf123456, v2           ::  v_dual_fmaak_f32     v6, v3, v1, 1.0
+// GFX11-NEXT:{{^}}                        ^
+
+v_dual_fmamk_f32    v122, 0xdeadbeef, 2, v161    ::  v_dual_fmamk_f32  v123, s0, 1, v162
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+// GFX11-NEXT:{{^}}v_dual_fmamk_f32    v122, 0xdeadbeef, 2, v161    ::  v_dual_fmamk_f32  v123, s0, 1, v162
+// GFX11-NEXT:{{^}}                          ^
+
+v_dual_fmamk_f32    v122, v1, 2, v161            ::  v_dual_fmamk_f32  v123, s0, 1, v162
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+// GFX11-NEXT:{{^}}v_dual_fmamk_f32    v122, v1, 2, v161            ::  v_dual_fmamk_f32  v123, s0, 1, v162
+// GFX11-NEXT:{{^}}                                                                                 ^
+
+//===----------------------------------------------------------------------===//
 // Check that assembler detects a different literal regardless of its location.
 //===----------------------------------------------------------------------===//
 
 v_dual_fmamk_f32    v122, 0xdeadbeef, 0xdeadbeef, v161 ::  v_dual_fmamk_f32  v123, 0xdeadbeef, 0x1234, v162
 // GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 // GFX11-NEXT:{{^}}v_dual_fmamk_f32    v122, 0xdeadbeef, 0xdeadbeef, v161 ::  v_dual_fmamk_f32  v123, 0xdeadbeef, 0x1234, v162
 // GFX11-NEXT:{{^}}                                                                                   ^
 

llvm/test/MC/AMDGPU/literals.s

 
 // NOGCN: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand (violates constant bus restrictions)
 v_madak_f32 v0, scc, v0, 0x11213141
 
 // NOGCN: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 v_madak_f32 v0, 0xff32ff, v0, 0x11213141
 
 // NOGCN: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+v_madak_f32 v0, 0xff32ff, v0, 1
+
+// NOGCN: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 v_madmk_f32 v0, 0xff32ff, 0x11213141, v0
 
+// NOGCN: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+v_madmk_f32 v0, 0xff32ff, -1, v0
+
 // NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 v_madak_f16 v0, 0xff32, v0, 0x1122
 
 // NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+v_madak_f16 v0, 0xff32, v0, 0
+
+// NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: instruction not supported on this GPU
+// NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
 v_madmk_f16 v0, 0xff32, 0x1122, v0
 
+// NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: instruction not supported on this GPU
+// NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: only one unique literal operand is allowed
+v_madmk_f16 v0, 0xff32, 1, v0
+
 // NOSICIVI: :[[@LINE+2]]:{{[0-9]+}}: error: register not available on this GPU
 // NOGFX9: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand (violates constant bus restrictions)
 v_cmp_eq_f32 s[0:1], private_base, private_limit
 
 // NOSICIVI: :[[@LINE+2]]:{{[0-9]+}}: error: register not available on this GPU
 // NOGFX9: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand (violates constant bus restrictions)
 v_cmp_eq_f32 s[0:1], private_base, s0