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

[Pierre van Houtryve via llvm-commits]

https://github.com/Pierre-vh updated https://github.com/llvm/llvm-project/pull/69296

>From ff4317ad4c91beccd1d2a400653947786598f2ff Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Tue, 17 Oct 2023 10:29:14 +0200
Subject: [PATCH 1/2] [TableGen] Handle duplicate rules in combiners

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.
---
 .../misc/redundant-combine-in-list.td         | 30 ++++++++++++
 .../TableGen/GlobalISelCombinerEmitter.cpp    | 48 ++++++++++++-------
 2 files changed, 60 insertions(+), 18 deletions(-)
 create mode 100644 llvm/test/TableGen/GlobalISelCombinerEmitter/misc/redundant-combine-in-list.td

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;
+    }
   }
 }
 

>From 53e1b297a0a06a9ae2a0f0b66aa145b741a85b77 Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Tue, 17 Oct 2023 10:49:00 +0200
Subject: [PATCH 2/2] format

---
 llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
index 11651526be8a152..91147b1dc0522c7 100644
--- a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
@@ -3637,14 +3637,16 @@ void GICombinerEmitter::gatherRules(
     }
 
     StringRef RuleName = Rec->getName();
-    if(!RulesSeen.insert(RuleName).second) {
-      PrintWarning(Rec->getLoc(), "skipping rule '" + Rec->getName() + "' because it has already been processed");
+    if (!RulesSeen.insert(RuleName).second) {
+      PrintWarning(Rec->getLoc(),
+                   "skipping rule '" + Rec->getName() +
+                       "' because it has already been processed");
       continue;
     }
 
     AllCombineRules.emplace_back(NextRuleID, Rec->getName().str());
     CombineRuleBuilder CRB(Target, SubtargetFeatures, *Rec, NextRuleID++,
-                            ActiveRules);
+                           ActiveRules);
 
     if (!CRB.parseAll()) {
       assert(ErrorsPrinted && "Parsing failed without errors!");