[AMDGPU][AsmParser] Do not use predicates for validation of NamedIntOperands. #90251

kosarev · 2024-04-26T19:09:18Z

Their job is to discriminate between different types of operands, not to check if they are valid. For validation we can use conversion functions.

Clears the road to generating predicates automatically.

Part of #62629.

llvmbot · 2024-04-26T19:09:51Z

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

Changes

Their job is to discriminate between different types of operands, not to check if they are valid. For validation we can use conversion functions.

Clears the road to generating predicates automatically.

Full diff: https://github.com/llvm/llvm-project/pull/90251.diff

4 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp (+8-8)
(modified) llvm/lib/Target/AMDGPU/SIInstrInfo.td (+26-10)
(modified) llvm/test/MC/AMDGPU/ds-err.s (+4-4)
(modified) llvm/test/MC/AMDGPU/gfx11_asm_err.s (+2-2)

diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index 38667235211471..5b94741ff77994 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -384,8 +384,8 @@ class AMDGPUOperand : public MCParsedAsmOperand {
   bool isIdxen() const { return isImmTy(ImmTyIdxen); }
   bool isAddr64() const { return isImmTy(ImmTyAddr64); }
   bool isOffset() const { return isImmTy(ImmTyOffset); }
-  bool isOffset0() const { return isImmTy(ImmTyOffset0) && isUInt<8>(getImm()); }
-  bool isOffset1() const { return isImmTy(ImmTyOffset1) && isUInt<8>(getImm()); }
+  bool isOffset0() const { return isImmTy(ImmTyOffset0); }
+  bool isOffset1() const { return isImmTy(ImmTyOffset1); }
   bool isSMEMOffsetMod() const { return isImmTy(ImmTySMEMOffsetMod); }
   bool isFlatOffset() const { return isImmTy(ImmTyOffset) || isImmTy(ImmTyInstOffset); }
   bool isGDS() const { return isImmTy(ImmTyGDS); }
@@ -8923,11 +8923,11 @@ bool AMDGPUOperand::isBLGP() const {
 }
 
 bool AMDGPUOperand::isCBSZ() const {
-  return isImm() && getImmTy() == ImmTyCBSZ && isUInt<3>(getImm());
+  return isImm() && getImmTy() == ImmTyCBSZ;
 }
 
 bool AMDGPUOperand::isABID() const {
-  return isImm() && getImmTy() == ImmTyABID && isUInt<4>(getImm());
+  return isImm() && getImmTy() == ImmTyABID;
 }
 
 bool AMDGPUOperand::isS16Imm() const {
@@ -9649,15 +9649,15 @@ bool AMDGPUOperand::isEndpgm() const { return isImmTy(ImmTyEndpgm); }
 //===----------------------------------------------------------------------===//
 
 bool AMDGPUOperand::isWaitVDST() const {
-  return isImmTy(ImmTyWaitVDST) && isUInt<4>(getImm());
+  return isImmTy(ImmTyWaitVDST);
 }
 
 bool AMDGPUOperand::isWaitVAVDst() const {
-  return isImmTy(ImmTyWaitVAVDst) && isUInt<4>(getImm());
+  return isImmTy(ImmTyWaitVAVDst);
 }
 
 bool AMDGPUOperand::isWaitVMVSrc() const {
-  return isImmTy(ImmTyWaitVMVSrc) && isUInt<1>(getImm());
+  return isImmTy(ImmTyWaitVMVSrc);
 }
 
 //===----------------------------------------------------------------------===//
@@ -9665,7 +9665,7 @@ bool AMDGPUOperand::isWaitVMVSrc() const {
 //===----------------------------------------------------------------------===//
 
 bool AMDGPUOperand::isWaitEXP() const {
-  return isImmTy(ImmTyWaitEXP) && isUInt<3>(getImm());
+  return isImmTy(ImmTyWaitEXP);
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.td b/llvm/lib/Target/AMDGPU/SIInstrInfo.td
index f1afbcc060b261..eb9bfa345188ec 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.td
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.td
@@ -1000,8 +1000,10 @@ def SDWAVopcDst : BoolRC {
 }
 
 class NamedIntOperand<ValueType Type, string Prefix, bit Optional = 1,
-                      string name = NAME, string ConvertMethod = "nullptr">
+                      string name = NAME>
     : CustomOperand<Type, Optional, name> {
+  string Validator = "[](int64_t V) { return true; }";
+  string ConvertMethod = "[](int64_t &V) { return "#Validator#"(V); }";
   let ParserMethod =
     "[this](OperandVector &Operands) -> ParseStatus { "#
     "return parseIntWithPrefix(\""#Prefix#"\", Operands, "#
@@ -1045,8 +1047,10 @@ class ArrayOperand0<string Id, string Name = NAME>
 let ImmTy = "ImmTyOffset" in
 def flat_offset : CustomOperand<i32, 1, "FlatOffset">;
 def Offset : NamedIntOperand<i32, "offset">;
+let Validator = "isUInt<8>" in {
 def Offset0 : NamedIntOperand<i8, "offset0">;
 def Offset1 : NamedIntOperand<i8, "offset1">;
+}
 
 def gds : NamedBitOperand<"gds", "GDS">;
 
@@ -1103,25 +1107,37 @@ let DefaultValue = "0xf" in {
 def DppRowMask : NamedIntOperand<i32, "row_mask">;
 def DppBankMask : NamedIntOperand<i32, "bank_mask">;
 }
-def DppBoundCtrl : NamedIntOperand<i1, "bound_ctrl", 1, "DppBoundCtrl",
-    "[this] (int64_t &BC) -> bool { return convertDppBoundCtrl(BC); }">;
+def DppBoundCtrl : NamedIntOperand<i1, "bound_ctrl"> {
+  let ConvertMethod = "[this] (int64_t &BC) -> bool { return convertDppBoundCtrl(BC); }";
+}
 
 let DecoderMethod = "decodeDpp8FI" in
 def Dpp8FI : NamedIntOperand<i32, "fi", 1, "DppFI">;
 def Dpp16FI : NamedIntOperand<i32, "fi", 1, "DppFI">;
 
 def blgp : CustomOperand<i32, 1, "BLGP">;
-def CBSZ : NamedIntOperand<i32, "cbsz">;
-def ABID : NamedIntOperand<i32, "abid">;
-
+def CBSZ : NamedIntOperand<i32, "cbsz"> {
+  let Validator = "isUInt<3>";
+}
+def ABID : NamedIntOperand<i32, "abid"> {
+  let Validator = "isUInt<4>";
+}
 def hwreg : CustomOperand<i32, 0, "Hwreg">;
 
 def exp_tgt : CustomOperand<i32, 0, "ExpTgt">;
 
-def WaitVDST : NamedIntOperand<i8, "wait_vdst">;
-def WaitEXP : NamedIntOperand<i8, "wait_exp">;
-def WaitVAVDst : NamedIntOperand<i8, "wait_va_vdst">;
-def WaitVMVSrc : NamedIntOperand<i8, "wait_vm_vsrc">;
+def WaitVDST : NamedIntOperand<i8, "wait_vdst"> {
+  let Validator = "isUInt<4>";
+}
+def WaitEXP : NamedIntOperand<i8, "wait_exp"> {
+  let Validator = "isUInt<3>";
+}
+def WaitVAVDst : NamedIntOperand<i8, "wait_va_vdst"> {
+  let Validator = "isUInt<4>";
+}
+def WaitVMVSrc : NamedIntOperand<i8, "wait_vm_vsrc"> {
+  let Validator = "isUInt<1>";
+}
 
 class KImmFPOperand<ValueType vt> : ImmOperand<vt> {
   let OperandNamespace = "AMDGPU";
diff --git a/llvm/test/MC/AMDGPU/ds-err.s b/llvm/test/MC/AMDGPU/ds-err.s
index 2d25fdf5e302d1..c31f4c7593950e 100644
--- a/llvm/test/MC/AMDGPU/ds-err.s
+++ b/llvm/test/MC/AMDGPU/ds-err.s
@@ -18,19 +18,19 @@ ds_write2_b32 v2, v4, v6 offset0:4 offset0:8
 ds_write2_b32 v2, v4, v6 offset1:4 offset1:8
 
 // offset0 too big
-// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand for instruction
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid offset0 value.
 ds_write2_b32 v2, v4, v6 offset0:1000000000
 
 // offset0 too big
-// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand for instruction
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid offset0 value.
 ds_write2_b32 v2, v4, v6 offset0:0x100
 
 // offset1 too big
-// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand for instruction
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid offset1 value.
 ds_write2_b32 v2, v4, v6 offset1:1000000000
 
 // offset1 too big
-// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand for instruction
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: invalid offset1 value.
 ds_write2_b32 v2, v4, v6 offset1:0x100
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/test/MC/AMDGPU/gfx11_asm_err.s b/llvm/test/MC/AMDGPU/gfx11_asm_err.s
index 3ec31626be5b70..7f99afe0192599 100644
--- a/llvm/test/MC/AMDGPU/gfx11_asm_err.s
+++ b/llvm/test/MC/AMDGPU/gfx11_asm_err.s
@@ -22,13 +22,13 @@ s_delay_alu instid0(VALU_DEP_1) | SALU_CYCLE_1)
 // GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: expected a left parenthesis
 
 lds_direct_load v15 wait_vdst:16
-// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: invalid wait_vdst value.
 
 lds_direct_load v15 wait_vdst
 // GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
 
 v_interp_p10_f32 v0, v1, v2, v3 wait_exp:8
-// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
+// GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: invalid wait_exp value.
 
 v_interp_p2_f32 v0, -v1, v2, v3 wait_exp
 // GFX11: :[[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction

github-actions · 2024-04-26T19:12:06Z

✅ With the latest revision this PR passed the C/C++ code formatter.

rampitec · 2024-04-26T20:01:50Z

llvm/lib/Target/AMDGPU/SIInstrInfo.td

-def ABID : NamedIntOperand<i32, "abid">;
-
+def CBSZ : NamedIntOperand<i32, "cbsz"> {
+  let Validator = "isUInt<3>";


isUInt<N> seems to be a common validator. Don't you want to generate default validator and supply N to the NamedIntOperand?

I wanted to not go that far in this patch and give it some time to settle and see how it works with other kinds of operands, like NamedBitOperand, etc. Shouldn't be hard to do that later, if needed?

OK, just an idea.

rampitec

LGTM. I like new error messages.

…perands. Their job is to discriminate between different types of operands, not to check if they are valid. For validation we can use conversion functions. Clears the road to generating predicates automatically. Part of <llvm#62629>.

kosarev requested review from jayfoad, arsenm and rampitec April 26, 2024 19:09

llvmbot added backend:AMDGPU mc Machine (object) code labels Apr 26, 2024

kosarev force-pushed the no-validation-in-predicates branch 2 times, most recently from ef9c90d to b7adca1 Compare April 26, 2024 19:16

rampitec reviewed Apr 26, 2024

View reviewed changes

rampitec approved these changes Apr 26, 2024

View reviewed changes

arsenm approved these changes Apr 27, 2024

View reviewed changes

kosarev force-pushed the no-validation-in-predicates branch from b7adca1 to 8cfa0cc Compare April 29, 2024 09:43

kosarev force-pushed the no-validation-in-predicates branch from 8cfa0cc to cb06d8d Compare April 29, 2024 09:49

kosarev merged commit e5c92c5 into llvm:main Apr 29, 2024
4 checks passed

kosarev deleted the no-validation-in-predicates branch April 29, 2024 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMDGPU][AsmParser] Do not use predicates for validation of NamedIntOperands. #90251

[AMDGPU][AsmParser] Do not use predicates for validation of NamedIntOperands. #90251

kosarev commented Apr 26, 2024 •

edited

Loading

llvmbot commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading

rampitec Apr 26, 2024

kosarev Apr 26, 2024

rampitec Apr 26, 2024

rampitec left a comment

[AMDGPU][AsmParser] Do not use predicates for validation of NamedIntOperands. #90251

[AMDGPU][AsmParser] Do not use predicates for validation of NamedIntOperands. #90251

Conversation

kosarev commented Apr 26, 2024 • edited Loading

llvmbot commented Apr 26, 2024 • edited Loading

github-actions bot commented Apr 26, 2024 • edited Loading

rampitec Apr 26, 2024

Choose a reason for hiding this comment

kosarev Apr 26, 2024

Choose a reason for hiding this comment

rampitec Apr 26, 2024

Choose a reason for hiding this comment

rampitec left a comment

Choose a reason for hiding this comment

kosarev commented Apr 26, 2024 •

edited

Loading

llvmbot commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading