[llvm] d2b74d7 - [TableGen] Handle duplicate rules in combiners (#69296)

[via llvm-commits]

Author: Pierre van Houtryve
Date: 2023-10-17T10:59:32+02:00
New Revision: d2b74d7e4217b03e9f127505fe42410ab096afe6

URL: https://github.com/llvm/llvm-project/commit/d2b74d7e4217b03e9f127505fe42410ab096afe6
DIFF: https://github.com/llvm/llvm-project/commit/d2b74d7e4217b03e9f127505fe42410ab096afe6.diff

LOG: [TableGen] Handle duplicate rules in combiners (#69296)

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.

Added: 
    llvm/test/TableGen/GlobalISelCombinerEmitter/misc/redundant-combine-in-list.td

Modified: 
    llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp

Removed: 
    


################################################################################
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 f6251cb67188538..7992cb4362a1718 100644
--- a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
@@ -3307,6 +3307,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);
@@ -3624,27 +3628,37 @@ 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;
+    }
   }
 }