[AMDGPU][AsmParser] Refine SMRD offset definitions.

[kosarev]

Summary

• Fixes the type of default 8-bit offset operands.
• Adds a test for optional offsets.

This is effectively an NFC.

[kosarev]

SP3 doesn't seem to allow these optional offsets and we don't have any tests for them, so this assumes they shouldn't be supported. But happy to rework to add the missing tests if we know otherwise.

[arsenm]

llvm/test/MC/AMDGPU/gfx7_asm_err.s

1 ↗

(On Diff #490889)

Should also include run lines for gfx6 and 8?

[foad]

"Forbid optional offsets" is a strange way to describe this. How about "make offset mandatory"?

Does the (LLVM or SP3) assembler allow the offset to be omitted on GFX8/9?

[dp]

The patch itself looks fine, but it breaks backward compatibility with existing code.
I think if you make offset mandatory for GFX6 and GFX7, the same should be done for other GPUs. But IMO preserving compatibility with existing code is more important.
BTW, have you noticed that SP3 does not allow 32-bit offsets with SMRD? LLVM assembler accepts these, but I do not know if HW can handle it.

[arsenm]

In D142231#4076375, @dp wrote:

The patch itself looks fine, but it breaks backward compatibility with existing code.
I think if you make offset mandatory for GFX6 and GFX7, the same should be done for other GPUs. But IMO preserving compatibility with existing code is more important.
BTW, have you noticed that SP3 does not allow 32-bit offsets with SMRD? LLVM assembler accepts these, but I do not know if HW can handle it.

I think that worked for CI only

[dp]

I think that worked for CI only

Yes, it works for SMRD only, not for SMEM. Not sure about SI.

[kosarev]

Reworked to keep optional offsets.

[kosarev]

@dp Ping.

[dp]

llvm/lib/Target/AMDGPU/SMInstructions.td

759 ↗

(On Diff #500237)

I assume making this operand optional or mandatory does not really affect anything, does it?

[kosarev]

llvm/lib/Target/AMDGPU/SMInstructions.td

759 ↗

(On Diff #500237)

isSMRDLiteralOffset() reads like it can't really be optional.

[dp]

LGTM

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

public:
   void cvtMubufAtomic(MCInst &Inst, const OperandVector &Operands) { cvtMubufImpl(Inst, Operands, true); }
   void cvtMtbuf(MCInst &Inst, const OperandVector &Operands);
 
   AMDGPUOperand::Ptr defaultCPol() const;
 
   AMDGPUOperand::Ptr defaultSMRDOffset8() const;
   AMDGPUOperand::Ptr defaultSMEMOffset() const;
   AMDGPUOperand::Ptr defaultSMEMOffsetMod() const;
-  AMDGPUOperand::Ptr defaultSMRDLiteralOffset() const;
   AMDGPUOperand::Ptr defaultFlatOffset() const;
 
   OperandMatchResultTy parseOModOperand(OperandVector &Operands);
 
   void cvtVOP3(MCInst &Inst, const OperandVector &Operands,
                OptionalImmIndexMap &OptionalIdx);
   void cvtVOP3OpSel(MCInst &Inst, const OperandVector &Operands);
   void cvtVOP3(MCInst &Inst, const OperandVector &Operands);

 
 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 isImmLiteral() && !isUInt<8>(getImm()) && isUInt<32>(getImm());
 }
 
 AMDGPUOperand::Ptr AMDGPUAsmParser::defaultSMRDOffset8() const {
-  return AMDGPUOperand::CreateImm(this, 0, SMLoc(), AMDGPUOperand::ImmTyOffset);
+  return AMDGPUOperand::CreateImm(this, 0, SMLoc(), AMDGPUOperand::ImmTyNone);
 }
 
 AMDGPUOperand::Ptr AMDGPUAsmParser::defaultSMEMOffset() const {
   return AMDGPUOperand::CreateImm(this, 0, SMLoc());
 }
 
 AMDGPUOperand::Ptr AMDGPUAsmParser::defaultSMEMOffsetMod() const {
   return AMDGPUOperand::CreateImm(this, 0, SMLoc(),
                                   AMDGPUOperand::ImmTySMEMOffsetMod);
 }
 
-AMDGPUOperand::Ptr AMDGPUAsmParser::defaultSMRDLiteralOffset() const {
-  return AMDGPUOperand::CreateImm(this, 0, SMLoc(), AMDGPUOperand::ImmTyOffset);
-}
-
 AMDGPUOperand::Ptr AMDGPUAsmParser::defaultFlatOffset() const {
   return AMDGPUOperand::CreateImm(this, 0, SMLoc(), AMDGPUOperand::ImmTyOffset);
 }
 
 //===----------------------------------------------------------------------===//
 // vop3
 //===----------------------------------------------------------------------===//
 

llvm/test/MC/AMDGPU/smrd.s

 // RUN: not llvm-mc -arch=amdgcn -show-encoding %s | FileCheck --check-prefix=GCN  %s
 // RUN: not llvm-mc -arch=amdgcn -mcpu=tahiti -show-encoding %s | FileCheck --check-prefix=GCN %s
 // RUN: llvm-mc -arch=amdgcn -mcpu=bonaire -show-encoding %s | FileCheck --check-prefixes=GCN,CI %s
 // RUN: not llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s | FileCheck --check-prefix=VI %s
 
 // RUN: not llvm-mc -arch=amdgcn %s 2>&1 | FileCheck %s --check-prefix=NOSI --implicit-check-not=error:
 // RUN: not llvm-mc -arch=amdgcn -mcpu=tahiti  %s 2>&1 | FileCheck %s --check-prefix=NOSI --implicit-check-not=error:
 // RUN: not llvm-mc -arch=amdgcn -mcpu=fiji  %s 2>&1 | FileCheck %s --check-prefix=NOVI --implicit-check-not=error:
 
 //===----------------------------------------------------------------------===//
 // Offset Handling
 //===----------------------------------------------------------------------===//
 
+// SP3 requires the immediate offset, but we allow to drop it for
+// compatibility reasons.
+s_load_dword s1, s[2:3]
+// GCN: s_load_dword s1, s[2:3], 0x0 ; encoding: [0x00,0x83,0x00,0xc0]
+// VI: s_load_dword s1, s[2:3], 0x0 ; encoding: [0x41,0x00,0x02,0xc0,0x00,0x00,0x00,0x00]
+
 s_load_dword s1, s[2:3], 0xfc
 // GCN: s_load_dword s1, s[2:3], 0xfc ; encoding: [0xfc,0x83,0x00,0xc0]
 // VI:  s_load_dword s1, s[2:3], 0xfc   ; encoding: [0x41,0x00,0x02,0xc0,0xfc,0x00,0x00,0x00]
 
 s_load_dword s1, s[2:3], 0xff
 // GCN: s_load_dword s1, s[2:3], 0xff ; encoding: [0xff,0x83,0x00,0xc0]
 // VI:  s_load_dword s1, s[2:3], 0xff   ; encoding: [0x41,0x00,0x02,0xc0,0xff,0x00,0x00,0x00]