[AMDGPU][AsmParser] Fix matching immediate literals.

[kosarev]

Summary

Prevents potential matching of literal offsets to non-literal operands.

[kosarev]

I'm not sure there's a way to prepare a reliable regression test for this.

[foad]

What does this fix?

[kosarev]

Update commit message.

[kosarev]

What does this fix?

The description should now hopefully answer this.

[arsenm]

In D142194#4068127, @kosarev wrote:

I'm not sure there's a way to prepare a reliable regression test for this.

If there's a literal parsing issue seems like it should be straightforward?

[kosarev]

If there's a literal parsing issue seems like it should be straightforward?

There is no reproducer, and even if we have one, it would still be not reliable, because the order in which the internal AsmParser machinery tries to match operands is generally beyond our control.

What's interesting about this change is that defaultSMRDOffset8() and defaultSMRDLiteralOffset() are not updated to add ImmTyNone operands instead of ImmTyOffset and we still pass the tests. That's because we don't really have any GFX6 tests with these operands dropped, so no default values used. (Addressed in D142231.)

[dp]

Does your patch fix this bug?

s_load_dword s5, s[2:3], glc

Assembler should trigger an error for GFX7, but glc is misinterpreted as offset operand.
I think your patch should fix this bug, so you may add this code as a test.

[kosarev]

Added the suggested test.

[kosarev]

Does your patch fix this bug?

s_load_dword s5, s[2:3], glc

It does, thanks! Is there a ticket to be mentioned in the commit message?

[dp]

Is there a ticket to be mentioned in the commit message?

There is no ticket. I found this bug while analyzing your patch and trying to break the parser.

[dp]

LGTM, but I suggest moving the test to some other place. *_unsupported.s are used solely to test identification of unsupported opcodes. gfx7_err_pos.s may be a better place for this test.

[kosarev]

Moved the test to gfx7_err_pos.s.

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

public:
   bool isSDWAFP32Operand() const;
   bool isSDWAInt16Operand() const;
   bool isSDWAInt32Operand() const;
 
   bool isImmTy(ImmTy ImmT) const {
     return isImm() && Imm.Type == ImmT;
   }
 
+  bool isImmLiteral() const { return isImmTy(ImmTyNone); }
+
   bool isImmModifier() const {
     return isImm() && Imm.Type != ImmTyNone;
   }
 
   bool isClampSI() const { return isImmTy(ImmTyClampSI); }
   bool isOModSI() const { return isImmTy(ImmTyOModSI); }
   bool isDMask() const { return isImmTy(ImmTyDMask); }
   bool isDim() const { return isImmTy(ImmTyDim); }
void AMDGPUAsmParser::cvtIntersectRay(MCInst &Inst,
   Inst.addOperand(MCOperand::createImm(1)); // a16
 }
 
 //===----------------------------------------------------------------------===//
 // smrd
 //===----------------------------------------------------------------------===//
 
 bool AMDGPUOperand::isSMRDOffset8() const {
-  return isImm() && isUInt<8>(getImm());
+  return isImmLiteral() && isUInt<8>(getImm());
 }
 
 bool AMDGPUOperand::isSMEMOffset() const {
   return isImmTy(ImmTyNone) ||
          isImmTy(ImmTyOffset); // Offset range is checked later by validator.
 }
 
 bool AMDGPUOperand::isSMRDLiteralOffset() const {
   // 32-bit literals are only supported on CI and we only want to use them
   // when the offset is > 8-bits.
-  return isImm() && !isUInt<8>(getImm()) && isUInt<32>(getImm());
+  return isImmLiteral() && !isUInt<8>(getImm()) && isUInt<32>(getImm());
 }
 
 AMDGPUOperand::Ptr AMDGPUAsmParser::defaultSMRDOffset8() const {
   return AMDGPUOperand::CreateImm(this, 0, SMLoc(), AMDGPUOperand::ImmTyOffset);
 }
 
 AMDGPUOperand::Ptr AMDGPUAsmParser::defaultSMEMOffset() const {
   return AMDGPUOperand::CreateImm(this, 0, SMLoc(), AMDGPUOperand::ImmTyOffset);
bool AMDGPUOperand::isCBSZ() const {
   return isImm() && getImmTy() == ImmTyCBSZ && isUInt<3>(getImm());
 }
 
 bool AMDGPUOperand::isABID() const {
   return isImm() && getImmTy() == ImmTyABID && isUInt<4>(getImm());
 }
 
 bool AMDGPUOperand::isS16Imm() const {
-  return isImm() && (isInt<16>(getImm()) || isUInt<16>(getImm()));
+  return isImmLiteral() && (isInt<16>(getImm()) || isUInt<16>(getImm()));
 }
 
 bool AMDGPUOperand::isU16Imm() const {
-  return isImm() && isUInt<16>(getImm());
+  return isImmLiteral() && isUInt<16>(getImm());
 }
 
 //===----------------------------------------------------------------------===//
 // dim
 //===----------------------------------------------------------------------===//
 
 bool AMDGPUAsmParser::parseDimId(unsigned &Encoding) {
   // We want to allow "dim:1D" etc.,

llvm/test/MC/AMDGPU/gfx7_unsupported.s

 v_subrev_f32_sdwa v255, v1, v2 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:DWORD
 // CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: sdwa variant of this instruction is not supported
 
 v_trunc_f32_sdwa v255, v1 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD
 // CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: sdwa variant of this instruction is not supported
 
 v_xor_b32_sdwa v255, v1, v2 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:DWORD
 // CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: sdwa variant of this instruction is not supported
+
+//===----------------------------------------------------------------------===//
+// cache policy is not supported for SMRD instructions
+//===----------------------------------------------------------------------===//
+
+s_load_dword s5, s[2:3], glc
+// CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: cache policy is not supported for SMRD instructions