[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

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

762

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

[kosarev]

llvm/lib/Target/AMDGPU/SMInstructions.td

762

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

[dp]

LGTM

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

public:
   int64_t parseGPRIdxMacro();
 
   void cvtMubuf(MCInst &Inst, const OperandVector &Operands) { cvtMubufImpl(Inst, Operands, false); }
   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 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 isImm() && !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);
 }
 
-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/lib/Target/AMDGPU/SMInstructions.td

 //===---- SMInstructions.td - Scalar Memory Instruction Definitions -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-def smrd_offset_8 : NamedOperandU32<"SMRDOffset8",
-                                  NamedMatchClass<"SMRDOffset8">> {
+def SMRDOffset8Class : NamedMatchClass<"SMRDOffset8", 0> {
+  let ParserMethod = "";
+}
+
+def smrd_offset_8 : NamedOperandU32<"SMRDOffset8", SMRDOffset8Class> {
   let OperandType = "OPERAND_IMMEDIATE";
 }
 
 class SMEMOffset : NamedOperandU32<"SMEMOffset",
                                    NamedMatchClass<"SMEMOffset">> {
   let OperandType = "OPERAND_IMMEDIATE";
   let EncoderMethod = "getSMEMOffsetEncoding";
   let DecoderMethod = "decodeSMEMOffset";

 
 defm S_DCACHE_DISCARD    : SM_Real_Discard_vi <0x28, "S_DCACHE_DISCARD">;
 defm S_DCACHE_DISCARD_X2 : SM_Real_Discard_vi <0x29, "S_DCACHE_DISCARD_X2">;
 
 //===----------------------------------------------------------------------===//
 // CI
 //===----------------------------------------------------------------------===//
 
+def SMRDLiteralOffsetClass : NamedMatchClass<"SMRDLiteralOffset", 0> {
+  let ParserMethod = "";
+}
+
 def smrd_literal_offset : NamedOperandU32<"SMRDLiteralOffset",
-                                          NamedMatchClass<"SMRDLiteralOffset">> {
+                                          SMRDLiteralOffsetClass> {
   let OperandType = "OPERAND_IMMEDIATE";
 }
 
 class SMRD_Real_Load_IMM_ci <bits<5> op, SM_Load_Pseudo ps> :
   SM_Real<ps>,
   Enc64 {
 
   let AssemblerPredicate = isGFX7Only;

llvm/test/MC/AMDGPU/gfx7_asm_err.s

+// RUN: not llvm-mc -arch=amdgcn -mcpu=bonaire %s 2>&1 | FileCheck %s --implicit-check-not=error:
+
+//==============================================================================
+// operands are not valid for this GPU or mode
+
+s_load_dword s1, s[2:3]
+// CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: operands are not valid for this GPU or mode