[TableGen] Gracefully error out in ParseTreePattern when DAG has zero operands so that llvm-tblgen doesn't crash

[pchaudhuri-nv]

Also handle the case when Pat->Child(i) is null in CodeGenDAGPatterns::FindPatternInputsAndOutputs().
Fixes issue #157619 : TableGen asserts on invalid cast

Tests run : make check-llvm, make check-llvm-tablegen

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-tablegen

Author: Prerona Chaudhuri (pchaudhuri-nv)

Changes

Fixes issue #157619 : TableGen asserts on invalid cast

---

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

2 Files Affected:

• (modified) llvm/test/TableGen/invalid-type-cast-patfrags.td (+4-4)
• (modified) llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp (+3-3)

diff --git a/llvm/test/TableGen/invalid-type-cast-patfrags.td b/llvm/test/TableGen/invalid-type-cast-patfrags.td
index 49d8a73b81078..fe63a71a17352 100644
--- a/llvm/test/TableGen/invalid-type-cast-patfrags.td
+++ b/llvm/test/TableGen/invalid-type-cast-patfrags.td
@@ -19,21 +19,21 @@ def INSTR_BAR : Instruction {
 }
 
 #ifdef ERROR1
-// ERROR1: [[@LINE+1]]:1: error: {{.*}} Invalid number of type casts!
+// ERROR1: error: Invalid number of type casts!
 def : Pat<([i32, i32, i32] (int_foo (i32 GPR32:$a))), ([i32, i32, i32] (INSTR_FOO $a))>;
 #endif
 
 #ifdef ERROR2
-// ERROR2: [[@LINE+1]]:1: error: {{.*}} Invalid number of type casts!
+// ERROR2: error: Invalid number of type casts!
 def : Pat<([]<ValueType> (int_bar)), ([]<ValueType> (INSTR_BAR))>;
 #endif
 
 #ifdef ERROR3
-// ERROR3: [[@LINE+1]]:1: error: {{.*}} Type cast only takes one operand!
+// ERROR3: error: Type cast only takes one operand!
 def : Pat<([i32, i32] (int_foo), (int_foo)), ([i32, i32] (INSTR_FOO))>;
 #endif
 
 #ifdef ERROR4
-// ERROR4: [[@LINE+1]]:1: error: {{.*}} Type cast should not have a name!
+// ERROR4: error: Type cast should not have a name!
 def : Pat<([i32, i32] ([i32, i32] (int_foo)):$name), ([i32, i32] (INSTR_FOO))>;
 #endif
diff --git a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
index 75bea77faba42..02b4cb78948e3 100644
--- a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
@@ -3014,10 +3014,10 @@ TreePatternNodePtr TreePattern::ParseTreePattern(const Init *TheInit,
 
   auto ParseCastOperand = [this](const DagInit *Dag, StringRef OpName) {
     if (Dag->getNumArgs() != 1)
-      error("Type cast only takes one operand!");
+      PrintFatalError("Type cast only takes one operand!");
 
     if (!OpName.empty())
-      error("Type cast should not have a name!");
+      PrintFatalError("Type cast should not have a name!");
 
     return ParseTreePattern(Dag->getArg(0), Dag->getArgNameStr(0));
   };
@@ -3029,7 +3029,7 @@ TreePatternNodePtr TreePattern::ParseTreePattern(const Init *TheInit,
 
     size_t NumTypes = New->getNumTypes();
     if (LI->empty() || LI->size() != NumTypes)
-      error("Invalid number of type casts!");
+      PrintFatalError("Invalid number of type casts!");
 
     // Apply the type casts.
     const CodeGenHwModes &CGH = getDAGPatterns().getTargetInfo().getHwModes();

[pchaudhuri-nv]

Requesting @arsenm , @jurahul to help review.

I am very new to this process, so a very small question, in the github pipeline windows and linux build is required, how do I complete that ? I have locally verified on linux that the lit tests are passing.

[pchaudhuri-nv]

Also, I have fixed only few of the asserts. If you want, I will change all the asserts to PrintFatalError. Kindly let me know.

Thank you

[arsenm]

Missing new test. The only test changes appear to be regressing error quality in covered cases. In general non-fatal errors are preferable. It's better to emit as many errors as possible

[arsenm]

This looks like a regression, the test no longer prints a line number?

[jurahul]

Right, because of the switch to PrintFatalError with no line info.

[jurahul]

I'd say this unit test should have no changes. It seems ParseCastOperand can return an optional<ParseTreePattern> instead so that it can gracefully fail when parsing fails, and then the caller code can handle this error so that it's not a fatal error.

[jurahul]

And use the example input from #157619 as a new unit test

[pchaudhuri-nv]

Thank you for the feedback. I have made the changes...

Also, I would like to highlight that AI agents have helped here. Kindly let me know if the changes are fine now.

[pchaudhuri-nv]

I was wondering if we can use PrintError instead or PrintFatalError, because ParseTreePattern does return nullptr even before my change. However for the test in the bug, FindPatternInputsAndOutputs then crashes because it doesn't handle the case when the children of Pat are nullptr.

Should I then also fix FindPatternInputsAndOutputs, so that we can use PrintError in ParseCastOperand ?

[arsenm]

That would probably be better

[pchaudhuri-nv]

Thank you for cooperation, I have updated the MR. Kindly let me know if this is good to be checked in.

[arsenm]

Error messages should start with a lowercase (although the other cases here also violate this policy). The phrasing is also an inconsistent style. Avoid referring to "This" and "it"

[pchaudhuri-nv]

Thank you for pointing this out. I have the changes locally, but looks like some issue with my github account and I am not able to push the changes :( . i am checking to fix this. Will update again.

Thank you for cooperation