[TableGen] Add assert to validate `Objects` list for `HwModeSelect` #123794

jurahul · 2025-01-21T18:04:16Z

Bail out of TableGen if any asserts fail before running the backend.
Add asserts to validate that the Objects and Modes lists for various HwModeSelect subclasses are of same length.
Eliminate equivalent check in CodeGenHWModes.cpp

topperc · 2025-01-22T00:45:46Z

Does this give a better user experience than the existing check in HwModeSelect::HwModeSelect?

jurahul · 2025-01-22T00:56:56Z

Does this give a better user experience than the existing check in HwModeSelect::HwModeSelect?

I didn't realize that the check was already there. I suspect it does not. With the assert, we are reusing the existing machinery in TableGen to do the check, whereas the existing check is custom code. May be its possible to consolidate the assert into the TableGen HWModeSelect class to prevent duplication and then delete the CPP one?

topperc · 2025-01-22T01:32:27Z

Does this give a better user experience than the existing check in HwModeSelect::HwModeSelect?

I didn't realize that the check was already there. I suspect it does not. With the assert, we are reusing the existing machinery in TableGen to do the check, whereas the existing check is custom code. May be its possible to consolidate the assert into the TableGen HWModeSelect class to prevent duplication and then delete the CPP one?

The Objects list has a different type in each subclass. Is it possible to pass the type to HwModeSelect somehow?

llvmbot · 2025-01-23T03:00:07Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-tablegen

Author: Rahul Joshi (jurahul)

Changes

Add asserts to validate that the Objects and Modes lists for various HwModeSelect subclasses are of same length.

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

6 Files Affected:

(modified) llvm/include/llvm/Target/Target.td (+10-6)
(modified) llvm/lib/TableGen/Main.cpp (+5)
(modified) llvm/lib/TableGen/TGParser.cpp (+7-4)
(modified) llvm/test/TableGen/BitsInit.td (+1-20)
(modified) llvm/test/TableGen/HwModeSelect.td (+3-2)
(modified) llvm/utils/TableGen/Common/CodeGenHwModes.cpp (-8)

diff --git a/llvm/include/llvm/Target/Target.td b/llvm/include/llvm/Target/Target.td
index 3e037affe1cfd2..ebe403c60c2b5e 100644
--- a/llvm/include/llvm/Target/Target.td
+++ b/llvm/include/llvm/Target/Target.td
@@ -43,8 +43,11 @@ def DefaultMode : HwMode<"", []>;
 // "Objects", which is a list of the same length as the list of modes.
 // The n-th element on the Objects list will be associated with the n-th
 // element on the Modes list.
-class HwModeSelect<list<HwMode> Ms> {
+class HwModeSelect<list<HwMode> Ms, int ObjectsLength> {
   list<HwMode> Modes = Ms;
+
+  assert !eq(ObjectsLength, !size(Modes)),
+     "The Objects and Modes lists must be the same length";
 }
 
 // A common class that implements a counterpart of ValueType, which is
@@ -53,7 +56,7 @@ class HwModeSelect<list<HwMode> Ms> {
 // objects could be used. This is specifically applicable to selection
 // patterns.
 class ValueTypeByHwMode<list<HwMode> Ms, list<ValueType> Ts>
-    : HwModeSelect<Ms>, ValueType<0, 0> {
+    : HwModeSelect<Ms, !size(Ts)>, ValueType<0, 0> {
   // The length of this list must be the same as the length of Ms.
   list<ValueType> Objects = Ts;
 }
@@ -64,7 +67,8 @@ class ValueTypeByHwMode<list<HwMode> Ms, list<ValueType> Ts>
 // or ValueType could be used. This is specifically applicable to selection
 // patterns.
 class PtrValueTypeByHwMode<ValueTypeByHwMode scalar, int addrspace>
-    : HwModeSelect<scalar.Modes>, PtrValueType<ValueType<0, 0>, addrspace> {
+    : HwModeSelect<scalar.Modes, !size(scalar.Objects)>, PtrValueType<ValueType<0, 0>, addrspace> {
+  // The length of this list must be the same as the length of Ms.
   list<ValueType> Objects = scalar.Objects;
 }
 
@@ -78,7 +82,7 @@ class RegInfo<int RS, int SS, int SA> {
 
 // The register size/alignment information, parameterized by a HW mode.
 class RegInfoByHwMode<list<HwMode> Ms = [], list<RegInfo> Ts = []>
-    : HwModeSelect<Ms> {
+    : HwModeSelect<Ms, !size(Ts)> {
   // The length of this list must be the same as the length of Ms.
   list<RegInfo> Objects = Ts;
 }
@@ -89,7 +93,7 @@ class SubRegRange<int size, int offset = 0> {
 }
 
 class SubRegRangeByHwMode<list<HwMode> Ms = [], list<SubRegRange> Ts = []>
-    : HwModeSelect<Ms> {
+    : HwModeSelect<Ms, !size(Ts)> {
   // The length of this list must be the same as the length of Ms.
   list<SubRegRange> Objects = Ts;
 }
@@ -574,7 +578,7 @@ class InstructionEncoding {
 // an EncodingByHwMode, its Inst and Size members are ignored and Ts are used
 // to encode and decode based on HwMode.
 class EncodingByHwMode<list<HwMode> Ms = [], list<InstructionEncoding> Ts = []>
-    : HwModeSelect<Ms> {
+    : HwModeSelect<Ms, !size(Ts)> {
   // The length of this list must be the same as the length of Ms.
   list<InstructionEncoding> Objects = Ts;
 }
diff --git a/llvm/lib/TableGen/Main.cpp b/llvm/lib/TableGen/Main.cpp
index 55a99cbfc58acd..35600bf2f1f861 100644
--- a/llvm/lib/TableGen/Main.cpp
+++ b/llvm/lib/TableGen/Main.cpp
@@ -128,6 +128,11 @@ int llvm::TableGenMain(const char *argv0,
     return 1;
   Timer.stopTimer();
 
+  // Return early if any other errors were generated during parsing
+  // (e.g., assert failures).
+  if (ErrorsPrinted > 0)
+    return reportError(argv0, Twine(ErrorsPrinted) + " errors.\n");
+
   // Write output to memory.
   Timer.startBackendTimer("Backend overall");
   std::string OutString;
diff --git a/llvm/lib/TableGen/TGParser.cpp b/llvm/lib/TableGen/TGParser.cpp
index d2115ab7627dad..ddd4279bf22f48 100644
--- a/llvm/lib/TableGen/TGParser.cpp
+++ b/llvm/lib/TableGen/TGParser.cpp
@@ -282,15 +282,18 @@ bool TGParser::SetValue(Record *CurRec, SMLoc Loc, const Init *ValName,
 
   if (OverrideDefLoc ? RV->setValue(V, Loc) : RV->setValue(V)) {
     std::string InitType;
-    if (const auto *BI = dyn_cast<BitsInit>(V))
+    bool LastSingleQuote = true;
+    if (const auto *BI = dyn_cast<BitsInit>(V)) {
       InitType = (Twine("' of type bit initializer with length ") +
                   Twine(BI->getNumBits())).str();
-    else if (const auto *TI = dyn_cast<TypedInit>(V))
+      LastSingleQuote = false;
+    } else if (const auto *TI = dyn_cast<TypedInit>(V)) {
       InitType = (Twine("' of type '") + TI->getType()->getAsString()).str();
+    }
     return Error(Loc, "Field '" + ValName->getAsUnquotedString() +
                           "' of type '" + RV->getType()->getAsString() +
-                          "' is incompatible with value '" +
-                          V->getAsString() + InitType + "'");
+                          "' is incompatible with value '" + V->getAsString() +
+                          InitType + (LastSingleQuote ? "'" : ""));
   }
   return false;
 }
diff --git a/llvm/test/TableGen/BitsInit.td b/llvm/test/TableGen/BitsInit.td
index c5527aebb94177..0e7fa7acb722f3 100644
--- a/llvm/test/TableGen/BitsInit.td
+++ b/llvm/test/TableGen/BitsInit.td
@@ -1,12 +1,10 @@
 
-// RUN: not llvm-tblgen %s 2>&1 > %t
-// RUN: FileCheck %s < %t
+// RUN: llvm-tblgen %s | FileCheck %s
 
 def a {
   bits<2> opc = { 0, 1 };
   bits<2> opc2 = { 1, 0 };
   bits<1> opc3 = { 1 };
-  bits<2> a = { opc, opc2 }; // error!
   bits<2> b = { opc{0}, opc2{0} };
   bits<2> c = { opc{1}, opc2{1} };
   bits<2> c = { opc3{0}, opc3 };
@@ -16,34 +14,25 @@ def a {
 // CHECK:   bits<2> opc = { 0, 1 };
 // CHECK:   bits<2> opc2 = { 1, 0 };
 // CHECK:   bits<1> opc3 = { 1 };
-// CHECK:   bits<2> a;
 // CHECK:   bits<2> b = { 1, 0 };
 // CHECK:   bits<2> c = { 1, 1 };
 // CHECK: }
 
 def {
-  bits<2> B1 = 0b011;  // bitfield is too small, reject
   bits<3> B2 = 0b011;  // ok
 
-  bits<2> C1 = 0b111;  // bitfield is too small, reject
   bits<3> C2 = 0b111;  // ok
 
   bits<2> D1 = { 0, 0 }; // ok
   bits<2> D2 = { 0b00 }; // ok
-  bits<3> D3 = { 0, 0 }; // type mismatch.  RHS doesn't have enough bits
-  bits<3> D4 = { 0b00 }; // type mismatch.  RHS doesn't have enough bits
   bits<1> D5 = { 0 };    // ok
   bits<1> D6 = { 1 };    // ok
-  bits<1> D7 = { 3 };    // type mismatch.  LHS doesn't have enough bits
-  bits<2> D8 = { 0 };    // type mismatch.  RHS doesn't have enough bits
 
   bits<8> E;
   let E{7...0} = {0,0,1,?,?,?,?,?};
   let E{3...0} = 0b0010;
 
   bits<8> F1 = { 0, 1, 0b1001, 0, 0b0 }; // ok
-  bits<7> F2 = { 0, 1, 0b1001, 0, 0b0 }; // LHS doesn't have enough bits
-  bits<9> F3 = { 0, 1, 0b1001, 0, 0b0 }; // RHS doesn't have enough bits
 
   bits<8> G1 = { 0, { 1, 0b1001, 0 }, 0b0 }; // ok
   bits<8> G2 = { 0, { 1, 0b1001 }, 0, 0b0 }; // ok
@@ -63,22 +52,14 @@ def {
 }
 
 // CHECK: def {{.*}} {
-// CHECK: bits<2> B1;
 // CHECK: bits<3> B2 = { 0, 1, 1 };
-// CHECK: bits<2> C1;
 // CHECK: bits<3> C2 = { 1, 1, 1 };
 // CHECK: bits<2> D1 = { 0, 0 };
 // CHECK: bits<2> D2 = { 0, 0 };
-// CHECK: bits<3> D3;
-// CHECK: bits<3> D4;
 // CHECK: bits<1> D5 = { 0 };
 // CHECK: bits<1> D6 = { 1 };
-// CHECK: bits<1> D7 = { !cast<bit>(3) };
-// CHECK: bits<2> D8;
 // CHECK: bits<8> E = { 0, 0, 1, ?, 0, 0, 1, 0 };
 // CHECK: bits<8> F1 = { 0, 1, 1, 0, 0, 1, 0, 0 };
-// CHECK: bits<7> F2;
-// CHECK: bits<9> F3;
 // CHECK: bits<8> G1 = { 0, 1, 1, 0, 0, 1, 0, 0 };
 // CHECK: bits<8> G2 = { 0, 1, 1, 0, 0, 1, 0, 0 };
 // CHECK: bits<8> G3 = { 0, 1, 1, 0, 0, 1, 0, 0 };
diff --git a/llvm/test/TableGen/HwModeSelect.td b/llvm/test/TableGen/HwModeSelect.td
index e849febe0c4cbf..0bac59a92304de 100644
--- a/llvm/test/TableGen/HwModeSelect.td
+++ b/llvm/test/TableGen/HwModeSelect.td
@@ -1,4 +1,4 @@
-// RUN: not --crash llvm-tblgen -gen-dag-isel -I %p/../../include %s 2>&1 | FileCheck %s
+// RUN: not llvm-tblgen -gen-dag-isel -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
 
 // The HwModeSelect class is intended to serve as a base class for other
 // classes that are then used to select a value based on the HW mode.
@@ -24,7 +24,8 @@ def HasFeat2 : Predicate<"Subtarget->hasFeat2()">;
 def TestMode1 : HwMode<"+feat1", [HasFeat1]>;
 def TestMode2 : HwMode<"+feat2", [HasFeat2]>;
 
+// CHECK: error: assertion failed: The Objects and Modes lists must be the same length
+// CHECK: [[FILE]]:[[@LINE+1]]:5: error: assertion failed in this record
 def BadDef : ValueTypeByHwMode<[TestMode1, TestMode2, DefaultMode],
                                [i8, i16, i32, i64]>;
 
-// CHECK: error: in record BadDef derived from HwModeSelect: the lists Modes and Objects should have the same size
diff --git a/llvm/utils/TableGen/Common/CodeGenHwModes.cpp b/llvm/utils/TableGen/Common/CodeGenHwModes.cpp
index c744691ae9e080..9996b5a4451f01 100644
--- a/llvm/utils/TableGen/Common/CodeGenHwModes.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenHwModes.cpp
@@ -41,14 +41,6 @@ void HwMode::dump() const { dbgs() << Name << ": " << Features << '\n'; }
 HwModeSelect::HwModeSelect(const Record *R, CodeGenHwModes &CGH) {
   std::vector<const Record *> Modes = R->getValueAsListOfDefs("Modes");
   std::vector<const Record *> Objects = R->getValueAsListOfDefs("Objects");
-  if (Modes.size() != Objects.size()) {
-    PrintError(
-        R->getLoc(),
-        "in record " + R->getName() +
-            " derived from HwModeSelect: the lists Modes and Objects should "
-            "have the same size");
-    report_fatal_error("error in target description.");
-  }
   for (auto [Mode, Object] : zip_equal(Modes, Objects)) {
     unsigned ModeId = CGH.getHwModeId(Mode);
     Items.emplace_back(ModeId, Object);

jurahul · 2025-01-23T17:47:31Z

The Objects list has a different type in each subclass. Is it possible to pass the type to HwModeSelect somehow?

Unfortunately, I could not find a way to pass the type, but I have made a change in how failing asserts are handled in the TableGen flow, which I think improves the user experience a bit. I'll start the review once the CI is green.

Currently, in the HwModeSelect.td which exercises this error case, tablegen prints an error message but does not terminate (in the C++ error check code) and later fails an assert in zip_equal (in HwModeSelect::HwModeSelect). With the new change, after the TG parser returns, we will detect that one of more asserts have failed, and avoid running any backend altogether, which is a better UE IMO (instead of seeing a crash and a stack trace).

jurahul · 2025-01-24T04:51:28Z

@topperc and @s-barannikov PTAL. I also considered splitting it into 2 changes: one for the TableGen main behavior and the HwModeSelect assert as a second one. If the overall change seems ok, I can do that

llvm/lib/TableGen/TGParser.cpp

- Add asserts to validate that the `Objects` and `Modes` lists for various `HwModeSelect` subclasses are of same length.

topperc

LGTM

jurahul changed the title ~~[NFC] Add assert to validate Objects list for HwModeSelect~~ [NFC] Add assert to validate Objects list for HwModeSelect Jan 21, 2025

jurahul requested review from topperc and s-barannikov January 22, 2025 00:17

jurahul force-pushed the target_td_HwModeSelect_assert branch from b4f2a71 to f665d25 Compare January 23, 2025 02:59

llvmbot added the tablegen label Jan 23, 2025

jurahul marked this pull request as draft January 23, 2025 02:59

jurahul force-pushed the target_td_HwModeSelect_assert branch 3 times, most recently from 1085d64 to fe5bc77 Compare January 23, 2025 17:41

jurahul marked this pull request as ready for review January 23, 2025 21:42

llvmbot added the mlir label Jan 23, 2025

jurahul changed the title ~~[NFC] Add assert to validate Objects list for HwModeSelect~~ [TableGen] Add assert to validate Objects list for HwModeSelect Jan 23, 2025

topperc reviewed Jan 24, 2025

View reviewed changes

llvm/lib/TableGen/TGParser.cpp Outdated Show resolved Hide resolved

[NFC] Add assert to validate Objects list for HwModeSelect

44fa7e9

- Add asserts to validate that the `Objects` and `Modes` lists for various `HwModeSelect` subclasses are of same length.

jurahul force-pushed the target_td_HwModeSelect_assert branch from fe5bc77 to 44fa7e9 Compare January 24, 2025 17:37

jurahul requested a review from topperc January 24, 2025 17:38

topperc approved these changes Jan 24, 2025

View reviewed changes

Merge branch 'main' into target_td_HwModeSelect_assert

5221907

jurahul merged commit aca08a8 into llvm:main Jan 27, 2025
8 checks passed

jurahul deleted the target_td_HwModeSelect_assert branch January 27, 2025 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TableGen] Add assert to validate `Objects` list for `HwModeSelect` #123794

[TableGen] Add assert to validate `Objects` list for `HwModeSelect` #123794

Uh oh!

jurahul commented Jan 21, 2025 •

edited

Loading

Uh oh!

topperc commented Jan 22, 2025

Uh oh!

jurahul commented Jan 22, 2025

Uh oh!

topperc commented Jan 22, 2025

Uh oh!

llvmbot commented Jan 23, 2025 •

edited

Loading

Uh oh!

jurahul commented Jan 23, 2025

Uh oh!

jurahul commented Jan 24, 2025

Uh oh!

Uh oh!

topperc left a comment

Uh oh!

Uh oh!

Uh oh!

[TableGen] Add assert to validate Objects list for HwModeSelect #123794

[TableGen] Add assert to validate Objects list for HwModeSelect #123794

Uh oh!

Conversation

jurahul commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Jan 22, 2025

Uh oh!

jurahul commented Jan 22, 2025

Uh oh!

topperc commented Jan 22, 2025

Uh oh!

llvmbot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jurahul commented Jan 23, 2025

Uh oh!

jurahul commented Jan 24, 2025

Uh oh!

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[TableGen] Add assert to validate `Objects` list for `HwModeSelect` #123794

[TableGen] Add assert to validate `Objects` list for `HwModeSelect` #123794

jurahul commented Jan 21, 2025 •

edited

Loading

llvmbot commented Jan 23, 2025 •

edited

Loading