[TableGen] Handle duplicate rules in combiners

[Pierre-vh]

We would crash when a rule was accidentally added twice to a combiner. This patch adds a warning instead to skip the already-processed rules.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

Author: Pierre van Houtryve (Pierre-vh)

Changes

We would crash when a rule was accidentally added twice to a combiner. This patch adds a warning instead to skip the already-processed rules.

---

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

2 Files Affected:

• (added) llvm/test/TableGen/GlobalISelCombinerEmitter/misc/redundant-combine-in-list.td (+30)
• (modified) llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp (+30-18)

diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/misc/redundant-combine-in-list.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/misc/redundant-combine-in-list.td
new file mode 100644
index 000000000000000..da38a228f672b0f
--- /dev/null
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/misc/redundant-combine-in-list.td
@@ -0,0 +1,30 @@
+// RUN: llvm-tblgen -I %p/../../../../include -gen-global-isel-combiner \
+// RUN:     -combiners=Combiner %s 2>&1 | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "llvm/Target/GlobalISel/Combine.td"
+
+// Check we don't crash if a combine is present twice in the list.
+
+def MyTargetISA : InstrInfo;
+def MyTarget : Target { let InstructionSet = MyTargetISA; }
+
+def dummy;
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: warning: skipping rule 'Foo' because it has already been processed
+def Foo : GICombineRule<
+  (defs root:$root),
+  (match (G_ZEXT $root, $x)),
+  (apply (G_TRUNC $root, $x))>;
+
+def Bar : GICombineRule<
+  (defs root:$root),
+  (match (G_TRUNC $root, $x)),
+  (apply (G_ZEXT $root, $x))>;
+
+def FooBar : GICombineGroup<[ Foo, Bar ]>;
+
+def Combiner: GICombiner<"GenMyCombiner", [
+  FooBar,
+  Foo
+]>;
diff --git a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
index 809415aeff153f7..11651526be8a152 100644
--- a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
@@ -3310,6 +3310,10 @@ class GICombinerEmitter final : public GlobalISelMatchTableExecutorEmitter {
   // combine rule used to disable/enable it.
   std::vector<std::pair<unsigned, std::string>> AllCombineRules;
 
+  // Keep track of all rules we've seen so far to ensure we don't process
+  // the same rule twice.
+  StringSet<> RulesSeen;
+
   MatchTable buildMatchTable(MutableArrayRef<RuleMatcher> Rules);
 
   void emitRuleConfigImpl(raw_ostream &OS);
@@ -3627,27 +3631,35 @@ void GICombinerEmitter::gatherRules(
     std::vector<RuleMatcher> &ActiveRules,
     const std::vector<Record *> &&RulesAndGroups) {
   for (Record *Rec : RulesAndGroups) {
-    if (Rec->isValueUnset("Rules")) {
-      AllCombineRules.emplace_back(NextRuleID, Rec->getName().str());
-      CombineRuleBuilder CRB(Target, SubtargetFeatures, *Rec, NextRuleID++,
-                             ActiveRules);
+    if (!Rec->isValueUnset("Rules")) {
+      gatherRules(ActiveRules, Rec->getValueAsListOfDefs("Rules"));
+      continue;
+    }
 
-      if (!CRB.parseAll()) {
-        assert(ErrorsPrinted && "Parsing failed without errors!");
-        continue;
-      }
+    StringRef RuleName = Rec->getName();
+    if(!RulesSeen.insert(RuleName).second) {
+      PrintWarning(Rec->getLoc(), "skipping rule '" + Rec->getName() + "' because it has already been processed");
+      continue;
+    }
 
-      if (StopAfterParse) {
-        CRB.print(outs());
-        continue;
-      }
+    AllCombineRules.emplace_back(NextRuleID, Rec->getName().str());
+    CombineRuleBuilder CRB(Target, SubtargetFeatures, *Rec, NextRuleID++,
+                            ActiveRules);
 
-      if (!CRB.emitRuleMatchers()) {
-        assert(ErrorsPrinted && "Emission failed without errors!");
-        continue;
-      }
-    } else
-      gatherRules(ActiveRules, Rec->getValueAsListOfDefs("Rules"));
+    if (!CRB.parseAll()) {
+      assert(ErrorsPrinted && "Parsing failed without errors!");
+      continue;
+    }
+
+    if (StopAfterParse) {
+      CRB.print(outs());
+      continue;
+    }
+
+    if (!CRB.emitRuleMatchers()) {
+      assert(ErrorsPrinted && "Emission failed without errors!");
+      continue;
+    }
   }
 }
 

[jayfoad]

Seems fine to me, though I am not familiar with this part of tablegen.

[github-actions]

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