Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TableGen] Added submulticlass typechecking to template arg values. #112904

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jofrn
Copy link
Contributor

@jofrn jofrn commented Oct 18, 2024

Some typechecking was missing when parsing a submulticlass reference. This adds it in and fixes any mistyped codes in .td files.

@llvmbot
Copy link
Member

llvmbot commented Oct 18, 2024

@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-backend-arm

Author: None (jofrn)

Changes

Some typechecking was missing when parsing a submulticlass reference. This adds it in and fixes any mistyped codes in .td files.


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

5 Files Affected:

  • (modified) llvm/lib/TableGen/TGParser.cpp (+6)
  • (modified) llvm/lib/Target/AMDGPU/VOPInstructions.td (+2-2)
  • (modified) llvm/lib/Target/ARM/ARMInstrMVE.td (+8-8)
  • (modified) llvm/lib/Target/ARM/ARMInstrNEON.td (+1-1)
  • (added) llvm/test/TableGen/submulticlass-typecheck.td (+12)
diff --git a/llvm/lib/TableGen/TGParser.cpp b/llvm/lib/TableGen/TGParser.cpp
index aed4f3fe0e96b5..de613f4f548395 100644
--- a/llvm/lib/TableGen/TGParser.cpp
+++ b/llvm/lib/TableGen/TGParser.cpp
@@ -812,6 +812,12 @@ ParseSubMultiClassReference(MultiClass *CurMC) {
     return Result;
   }
 
+  if (CheckTemplateArgValues(Result.TemplateArgs, Result.RefRange.Start,
+                             &Result.MC->Rec)) {
+    Result.MC = nullptr; // Error checking value list.
+    return Result;
+  }
+
   Result.RefRange.End = Lex.getLoc();
 
   return Result;
diff --git a/llvm/lib/Target/AMDGPU/VOPInstructions.td b/llvm/lib/Target/AMDGPU/VOPInstructions.td
index 05a7d907d237ae..3ccf78cc57b43f 100644
--- a/llvm/lib/Target/AMDGPU/VOPInstructions.td
+++ b/llvm/lib/Target/AMDGPU/VOPInstructions.td
@@ -37,7 +37,7 @@ defvar VOPDX_Max_Index = 12;
 
 class VOPD_Component<bits<5> OpIn, string vOPDName> {
   Instruction BaseVOP = !cast<Instruction>(NAME);
-  string VOPDName = "v_dual_" # !substr(vOPDName, 2);
+  string VOPDName = "v_dual_" # !if(!le(!size(vOPDName), 2), "", !substr(vOPDName, 2));
   bits<5> VOPDOp = OpIn;
   bit CanBeVOPDX = !le(VOPDOp, VOPDX_Max_Index);
 }
@@ -1705,4 +1705,4 @@ def VOPTrue16Table : GenericTable {
 
   let PrimaryKey = ["Opcode"];
   let PrimaryKeyName = "getTrue16OpcodeHelper";
-}
\ No newline at end of file
+}
diff --git a/llvm/lib/Target/ARM/ARMInstrMVE.td b/llvm/lib/Target/ARM/ARMInstrMVE.td
index 12c3968b9cecea..c3c4963a5da288 100644
--- a/llvm/lib/Target/ARM/ARMInstrMVE.td
+++ b/llvm/lib/Target/ARM/ARMInstrMVE.td
@@ -1998,7 +1998,7 @@ class MVE_VQxDMULH_Base<string iname, string suffix, bits<2> size, bit rounding,
 def MVEvqdmulh : SDNode<"ARMISD::VQDMULH", SDTIntBinOp>;
 
 multiclass MVE_VQxDMULH_m<string iname, MVEVectorVTInfo VTI,
-                      SDNode Op, Intrinsic unpred_int, Intrinsic pred_int,
+                      SDPatternOperator Op, Intrinsic unpred_int, Intrinsic pred_int,
                       bit rounding> {
   def "" : MVE_VQxDMULH_Base<iname, VTI.Suffix, VTI.Size, rounding>;
   defvar Inst = !cast<Instruction>(NAME);
@@ -2199,7 +2199,7 @@ def subnsw : PatFrag<(ops node:$lhs, node:$rhs),
 }]>;
 
 multiclass MVE_VRHADD_m<MVEVectorVTInfo VTI, SDNode Op,
-                      SDNode unpred_op, Intrinsic PredInt> {
+                      DefaultAttrsIntrinsic unpred_op, Intrinsic PredInt> {
   def "" : MVE_VRHADD_Base<VTI.Suffix, VTI.Unsigned, VTI.Size>;
   defvar Inst = !cast<Instruction>(NAME);
   defm : MVE_TwoOpPattern<VTI, Op, PredInt, (? (i32 VTI.Unsigned)), !cast<Instruction>(NAME)>;
@@ -2303,7 +2303,7 @@ class MVE_VHSUB_<string suffix, bit U, bits<2> size,
   : MVE_VHADDSUB<"vhsub", suffix, U, 0b1, size, pattern>;
 
 multiclass MVE_VHADD_m<MVEVectorVTInfo VTI, SDNode Op,
-                      SDNode unpred_op, Intrinsic PredInt, PatFrag add_op,
+                      DefaultAttrsIntrinsic unpred_op, Intrinsic PredInt, PatFrag add_op,
                       SDNode shift_op> {
   def "" : MVE_VHADD_<VTI.Suffix, VTI.Unsigned, VTI.Size>;
   defvar Inst = !cast<Instruction>(NAME);
@@ -2335,7 +2335,7 @@ defm MVE_VHADDu16 : MVE_VHADD<MVE_v8u16, avgflooru, addnuw, ARMvshruImm>;
 defm MVE_VHADDu32 : MVE_VHADD<MVE_v4u32, avgflooru, addnuw, ARMvshruImm>;
 
 multiclass MVE_VHSUB_m<MVEVectorVTInfo VTI,
-                      SDNode unpred_op, Intrinsic pred_int, PatFrag sub_op,
+                      DefaultAttrsIntrinsic unpred_op, Intrinsic pred_int, PatFrag sub_op,
                       SDNode shift_op> {
   def "" : MVE_VHSUB_<VTI.Suffix, VTI.Unsigned, VTI.Size>;
   defvar Inst = !cast<Instruction>(NAME);
@@ -4794,7 +4794,7 @@ class MVE_VxMULH<string iname, string suffix, bit U, bits<2> size, bit round,
   let validForTailPredication = 1;
 }
 
-multiclass MVE_VxMULH_m<string iname, MVEVectorVTInfo VTI, SDNode unpred_op,
+multiclass MVE_VxMULH_m<string iname, MVEVectorVTInfo VTI, DefaultAttrsIntrinsic unpred_op,
                         Intrinsic PredInt, bit round> {
   def "" : MVE_VxMULH<iname, VTI.Suffix, VTI.Unsigned, VTI.Size, round>;
   defvar Inst = !cast<Instruction>(NAME);
@@ -5370,8 +5370,8 @@ class MVE_VxADDSUB_qr<string iname, string suffix,
   let validForTailPredication = 1;
 }
 
-multiclass MVE_VHADDSUB_qr_m<string iname, MVEVectorVTInfo VTI, bit subtract, SDNode Op,
-                             Intrinsic unpred_int, Intrinsic pred_int, PatFrag add_op, PatFrag shift_op> {
+multiclass MVE_VHADDSUB_qr_m<string iname, MVEVectorVTInfo VTI, bit subtract, SDPatternOperator Op,
+                             Intrinsic unpred_int, Intrinsic pred_int, PatFrag add_op, SDNode shift_op> {
   def "" : MVE_VxADDSUB_qr<iname, VTI.Suffix, VTI.Unsigned, VTI.Size, subtract, VTI.Size>;
   defm : MVE_TwoOpPatternDup<VTI, Op, pred_int, (? (i32 VTI.Unsigned)), !cast<Instruction>(NAME)>;
   defm : MVE_vec_scalar_int_pat_m<!cast<Instruction>(NAME),
@@ -5576,7 +5576,7 @@ class MVE_VxxMUL_qr<string iname, string suffix,
 }
 
 multiclass MVE_VxxMUL_qr_m<string iname, MVEVectorVTInfo VTI, bit bit_28,
-                           PatFrag Op, Intrinsic int_unpred, Intrinsic int_pred> {
+                           SDPatternOperator Op, Intrinsic int_unpred, Intrinsic int_pred> {
   def "" : MVE_VxxMUL_qr<iname, VTI.Suffix, bit_28, VTI.Size, VTI.Size>;
 
   let Predicates = [HasMVEInt] in {
diff --git a/llvm/lib/Target/ARM/ARMInstrNEON.td b/llvm/lib/Target/ARM/ARMInstrNEON.td
index 48dcbdb137123a..20c52206fd3cd6 100644
--- a/llvm/lib/Target/ARM/ARMInstrNEON.td
+++ b/llvm/lib/Target/ARM/ARMInstrNEON.td
@@ -4906,7 +4906,7 @@ let Predicates = [HasMatMulInt8] in {
   }
 
   multiclass SUDOTLane<bit Q, RegisterClass RegTy, ValueType AccumTy, ValueType InputTy, dag RHS>
-        : N3VMixedDotLane<Q, 1, "vsudot", "u8", RegTy, AccumTy, InputTy, null_frag, null_frag> {
+        : N3VMixedDotLane<Q, 1, "vsudot", "u8", RegTy, AccumTy, InputTy, null_frag, (ins)> {
     def : Pat<
       (AccumTy (int_arm_neon_usdot (AccumTy RegTy:$Vd),
                                    (InputTy (bitconvert (AccumTy
diff --git a/llvm/test/TableGen/submulticlass-typecheck.td b/llvm/test/TableGen/submulticlass-typecheck.td
new file mode 100644
index 00000000000000..2a1f7ab094f881
--- /dev/null
+++ b/llvm/test/TableGen/submulticlass-typecheck.td
@@ -0,0 +1,12 @@
+// RUN: not llvm-tblgen %s 2>&1 | FileCheck %s
+// XFAIL: vg_leak
+// CHECK: {{.*}}: error: Value specified for template argument 'B::op' is of type bits<4>; expected type bits<8>: C::op
+// CHECK-NEXT: multiclass C<bits<4> op> : B<op>;
+class A<bits<8> op> {
+  bits<8> f = op;
+}
+multiclass B<bits<8> op> {
+  def : A<op>;
+}
+multiclass C<bits<4> op> : B<op>;
+defm D : C<0>;

@jofrn
Copy link
Contributor Author

jofrn commented Oct 18, 2024

This resolves #84910.

@@ -37,7 +37,7 @@ defvar VOPDX_Max_Index = 12;

class VOPD_Component<bits<5> OpIn, string vOPDName> {
Instruction BaseVOP = !cast<Instruction>(NAME);
string VOPDName = "v_dual_" # !substr(vOPDName, 2);
string VOPDName = "v_dual_" # !if(!le(!size(vOPDName), 2), "", !substr(vOPDName, 2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this change related to template arg type checking?

Copy link
Contributor Author

@jofrn jofrn Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A def within a multiclass inherits from VOPD_Component. This multiclass itself is a submulticlass, so CheckTemplateArgValues will be invoked on it.

vOPDName is passed as an empty string into here from multiclass VOP2eInst, which invokes the aforementioned multiclass, and the typechecking caught this since fields are also resolved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since fields are also resolved

That's the part I don't understand. It seems like CheckTemplateArgValues must do more than just checking types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it seems like it is changing some behaviour. If I dump all the records from AMDGPU.td (including your change to VOPInstructions.td) before and after your TGParser.cpp change, there are differences.

Copy link
Contributor Author

@jofrn jofrn Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in the dynamic nature of TableGen. Typechecking resolves fields when it needs to do so. Adding CheckTemplateArgValues to this location mirrors the location it is at within ParseSubClassReference, so fields are also resolved when parsing non-multi sub classes. The def within the multiclass forces instantiation of VOPD_Component.

@jayfoad
Copy link
Contributor

jayfoad commented Oct 18, 2024

I think there is something weirder going on here, at least in the AMDGPU case. Consider:

class c {
  int cc = 0;
}
class d {
  int dd = 0;
}
multiclass m {
  def NAME : d;
}
defm a : m, c; // works!!!
defm b : c, m; // fails with error: Couldn't find multiclass 'c'

Is it ever supposed to be valid to use defm with a class name, like c in defm a : m, c; above? Because that's exactly what we're doing here:

defm NAME : VOP2Inst_e32<opName, P, node, revOp>,

multiclass
    VOP2Inst_e32_VOPD<string opName, VOPProfile P, bits<5> VOPDOp,
                      string VOPDName, SDPatternOperator node = null_frag,
                      string revOp = opName> {
  defm NAME : VOP2Inst_e32<opName, P, node, revOp>,
              VOPD_Component<VOPDOp, VOPDName>;
}

where VOPD_Component is a class.

@jayfoad
Copy link
Contributor

jayfoad commented Oct 18, 2024

Is it ever supposed to be valid to use defm with a class name, like c in defm a : m, c; above?

Oh, I see this is allowed and documented. Sorry for the noise.

@arsenm arsenm requested a review from jurahul October 19, 2024 03:11
@@ -0,0 +1,12 @@
// RUN: not llvm-tblgen %s 2>&1 | FileCheck %s
// XFAIL: vg_leak
// CHECK: {{.*}}: error: Value specified for template argument 'B::op' is of type bits<4>; expected type bits<8>: C::op
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check the line number in the message

@@ -319,7 +319,7 @@ multiclass
multiclass
VOP2eInst<string opName, VOPProfile P, SDPatternOperator node = null_frag,
string revOp = opName, bit useSGPRInput = !eq(P.NumSrcArgs, 3)>
: VOP2eInst_Base<opName, P, -1, "", node, revOp, useSGPRInput>;
: VOP2eInst_Base<opName, P, -1, "v_cndmask_b16", node, revOp, useSGPRInput>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can pass vOPDName explicitly here in order to give this "fake" record more information, defm V_CNDMASK_B16 : VOP2eInst <"v_cndmask_b16", VOP2e_I16_I16_I16_I1_fake16>;. I do not see it as necessary though. The opcode is used in one location and it would not have been able to query this information before this change anyway. VOPDOp is given a value of -1, indicating a fake record as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow why this is hardcoding v_cndmask_b16

Copy link
Contributor Author

@jofrn jofrn Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string VOPDName = "v_dual_" # !substr(vOPDName, 2);

Otherwise the empty string will segfault on !substr. We can do this or check the indices. Also, the class is only used in this one location:
defm V_CNDMASK_B16 : VOP2eInst <"v_cndmask_b16", VOP2e_I16_I16_I16_I1_fake16>;

Copy link
Contributor

@arsenm arsenm Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

substr segfaults on an empty string!? That's a really bad tablegen bug. Should just fix that (or at least just use some filler string and not a real opcode name)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, replaced it with v_ instead. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

substr segfaults on an empty string!? That's a really bad tablegen bug.

@jofrn if you have time can fix this too please (in a separate patch).

Copy link
Contributor Author

@jofrn jofrn Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I merged at this point and fixed the -1 -> 0 switch you made in the last commit.

@jayfoad
Copy link
Contributor

jayfoad commented Oct 21, 2024

The AMDGPU changes highlight a change in behaviour caused by your patch. Here's a simple example:

class C {
  bit x;
}
multiclass M1<bits<8> Val> {
  let x = !eq(Val, -1) in def NAME : C;
}
multiclass M2 : M1<-1>;
defm X : M2;

Before your patch I get:

def X {	// C
  bit x = 1;
}

After your patch I get:

def X {	// C
  bit x = 0;
}

Why did this change? Is the new behaviour more correct than the old behaviour? Can you please also add something like this as a test case?

In any case the AMDGPU code should probably be changed not to rely on equality of a bits<5> value and -1.

@jofrn
Copy link
Contributor Author

jofrn commented Oct 22, 2024

Uploading a test to show that one needs a !cast to perform the correct check. In the before case, -1 is resolved to a bits<8> before M1 is resolved (and so x = 1 since Val and -1 as bits<8> are equal). In the after case, -1 is resolved to an integer, of which it is before any cast, so then it makes sense for it to require a cast into a bits.

It is peculiar that in the before case, X0's x = 0, meaning that -1 being casted to a bits<8> is not equal to Val, which I'd say is incorrect:

multiclass M0<bits<8> Val> {
  let x = !eq(Val, !cast<bits<8>>(-1)) in def NAME : C;
}

(in the after case, x is 1 as expected.)

@jofrn jofrn force-pushed the tblgen-submc branch 3 times, most recently from f55f2eb to 89a6cae Compare October 22, 2024 02:08
// RUN: llvm-tblgen %s | FileCheck %s
// XFAIL: vg_leak
// CHECK: def X0 { // C
// CHECK-NEXT: bit x = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bit x = 0; in the before case, yet !cast<bits<8>>(-1) is identical to Val.

let x = !eq(Val, !cast<bits<8>>(-1)) in def NAME : C;
}
multiclass M1<bits<8> Val> {
let x = !eq(Val, -1) in def NAME : C;
Copy link
Contributor Author

@jofrn jofrn Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should evaluate to bit x = 0 because -1 is an integer and Val is 255.

@jayfoad
Copy link
Contributor

jayfoad commented Oct 22, 2024

Thanks for the updates. The change in behaviour makes sense to me, although it seems like CheckTemplateArgValues is doing more than just checking -- it is actually casting template arguments to the declared type, which can change their values.

The AMDGPU fix still doesn't look right. The intention of the code is clearly that when you pass in -1, the !eq(..., -1) test should be true.

jayfoad added a commit to jayfoad/llvm-project that referenced this pull request Oct 22, 2024
Tweak VOP2eInst_Base so that it does not rely on !eq comparing an int
value (-1) with a bits<5> value. This is to avoid a change in behaviour
when llvm#112904 lands, which is a bug fix which has the side effect of
implicitly casting template arguments to the declared template parameter
type.
@jayfoad
Copy link
Contributor

jayfoad commented Oct 22, 2024

The AMDGPU fix still doesn't look right. The intention of the code is clearly that when you pass in -1, the !eq(..., -1) test should be true.

How about #113279 to completely avoid this AMDGPU problem?

jayfoad added a commit that referenced this pull request Oct 22, 2024
Tweak VOP2eInst_Base so that it does not rely on !eq comparing an int
value (-1) with a bits<5> value. This is to avoid a change in behaviour
when #112904 lands, which is a bug fix which has the side effect of
implicitly casting template arguments to the declared template parameter
type.
@jofrn jofrn force-pushed the tblgen-submc branch 3 times, most recently from 3f652b7 to 878fe7b Compare October 22, 2024 12:13
Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the ARM parts but the rest LGTM.

llvm/lib/Target/AMDGPU/VOPInstructions.td Outdated Show resolved Hide resolved
@jofrn jofrn changed the title [TableGen] Added submulticlass typechecking to template arg values. [TableGen][ARM] Added submulticlass typechecking to template arg values. Oct 22, 2024
@jofrn jofrn changed the title [TableGen][ARM] Added submulticlass typechecking to template arg values. [TableGen] Added submulticlass typechecking to template arg values. Oct 22, 2024
jofrn added a commit to jofrn/llvm-project that referenced this pull request Oct 22, 2024
llvm#112904 adds typechecking to submulticlass arguments, and these ones
were mistyped beforehand.
jofrn added a commit that referenced this pull request Oct 22, 2024
#112904 will add typechecking to submulticlass arguments, and these
ones are currently mistyped.
jofrn added 7 commits October 22, 2024 18:22
Some typechecking was missing when parsing a submulticlass reference.
This adds it in and fixes any mistyped codes in .td files.
The record now looks like:

  def V_CNDMASK_B16_e32 {
    ...
    Instruction BaseVOP = V_CNDMASK_B16_e32;
    string VOPDName = "v_dual_";
    bits<5> VOPDOp = { 1, 1, 1, 1, 1 };
    bit CanBeVOPDX = 0;
    -->
    Instruction BaseVOP = V_CNDMASK_B16_e32;
    string VOPDName = "v_dual_cndmask_b16";
    bits<5> VOPDOp = { 1, 1, 1, 1, 1 };
    bit CanBeVOPDX = 0;
  }

Whereas before, these fields were omitted entirely.
Now the record has this field as before, and the fake part is included
in the record:

  string VOPDName = "v_dual_";
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants