[TableGen] Split DAGISelMatcherOpt FactorNodes into 2 functions. NFC

[topperc]

The loop at the top of FactorNodes creates additional variables to deal with needing to use a pointer to a unique_ptr instead of a reference. Encapsulate this to its own function for better scoping.

This also allows us to directly skip this loop when we already know we have a ScopeMatcher.

I still hate the unique_ptr management in this code.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-tablegen

Author: Craig Topper (topperc)

Changes

The loop at the top of FactorNodes creates additional variables to deal with needing to use a pointer to a unique_ptr instead of a reference. Encapsulate this to its own function for better scoping.

This also allows us to directly skip this loop when we already know we have a ScopeMatcher.

I still hate the unique_ptr management in this code.

---

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

1 Files Affected:

• (modified) llvm/utils/TableGen/DAGISelMatcherOpt.cpp (+31-30)

diff --git a/llvm/utils/TableGen/DAGISelMatcherOpt.cpp b/llvm/utils/TableGen/DAGISelMatcherOpt.cpp
index f747944543cfd0..5193b3c0741937 100644
--- a/llvm/utils/TableGen/DAGISelMatcherOpt.cpp
+++ b/llvm/utils/TableGen/DAGISelMatcherOpt.cpp
@@ -191,34 +191,10 @@ static Matcher *FindNodeWithKind(Matcher *M, Matcher::KindTy Kind) {
   return nullptr;
 }
 
-/// FactorNodes - Turn matches like this:
-///   Scope
-///     OPC_CheckType i32
-///       ABC
-///     OPC_CheckType i32
-///       XYZ
-/// into:
-///   OPC_CheckType i32
-///     Scope
-///       ABC
-///       XYZ
-///
-static void FactorNodes(std::unique_ptr<Matcher> &InputMatcherPtr) {
-  // Look for a push node. Iterates instead of recurses to reduce stack usage.
-  ScopeMatcher *Scope = nullptr;
-  std::unique_ptr<Matcher> *RebindableMatcherPtr = &InputMatcherPtr;
-  while (!Scope) {
-    // If we reached the end of the chain, we're done.
-    Matcher *N = RebindableMatcherPtr->get();
-    if (!N)
-      return;
-
-    // If this is not a push node, just scan for one.
-    Scope = dyn_cast<ScopeMatcher>(N);
-    if (!Scope)
-      RebindableMatcherPtr = &(N->getNextPtr());
-  }
-  std::unique_ptr<Matcher> &MatcherPtr = *RebindableMatcherPtr;
+static void FactorNodes(std::unique_ptr<Matcher> &InputMatcherPtr);
+
+static void FactorScope(std::unique_ptr<Matcher> &MatcherPtr) {
+  ScopeMatcher *Scope = cast<ScopeMatcher>(MatcherPtr.get());
 
   // Okay, pull together the children of the scope node into a vector so we can
   // inspect it more easily.
@@ -353,7 +329,7 @@ static void FactorNodes(std::unique_ptr<Matcher> &InputMatcherPtr) {
       Shared->setNext(new ScopeMatcher(std::move(EqualMatchers)));
 
       // Recursively factor the newly created node.
-      FactorNodes(Shared->getNextPtr());
+      FactorScope(Shared->getNextPtr());
     }
 
     // Put the new Matcher where we started in OptionsToMatch.
@@ -470,7 +446,7 @@ static void FactorNodes(std::unique_ptr<Matcher> &InputMatcherPtr) {
     for (auto &M : Cases) {
       if (ScopeMatcher *SM = dyn_cast<ScopeMatcher>(M.second)) {
         std::unique_ptr<Matcher> Scope(SM);
-        FactorNodes(Scope);
+        FactorScope(Scope);
         M.second = Scope.release();
         assert(M.second && "null matcher");
       }
@@ -492,6 +468,31 @@ static void FactorNodes(std::unique_ptr<Matcher> &InputMatcherPtr) {
     Scope->resetChild(i, OptionsToMatch[i]);
 }
 
+/// FactorNodes - Turn matches like this:
+///   Scope
+///     OPC_CheckType i32
+///       ABC
+///     OPC_CheckType i32
+///       XYZ
+/// into:
+///   OPC_CheckType i32
+///     Scope
+///       ABC
+///       XYZ
+///
+static void FactorNodes(std::unique_ptr<Matcher> &InputMatcherPtr) {
+  // Look for a scope matcher. Iterates instead of recurses to reduce stack
+  // usage.
+  std::unique_ptr<Matcher> *MatcherPtr = &InputMatcherPtr;
+  do {
+    if (isa<ScopeMatcher>(*MatcherPtr))
+      return FactorScope(*MatcherPtr);
+
+    // If this is not a scope matcher, go to the next node.
+    MatcherPtr = &(MatcherPtr->get()->getNextPtr());
+  } while (MatcherPtr->get());
+}
+
 void llvm::OptimizeMatcher(std::unique_ptr<Matcher> &MatcherPtr,
                            const CodeGenDAGPatterns &CGP) {
   ContractNodes(MatcherPtr, CGP);

[s-barannikov]

LGTM

[arsenm]

I don't see why this is referencing unique_ptr at all. This could be done in terms of Matcher* or Matcher&

[topperc]

The code change the contents of the unique_ptr by calling unique_ptr::reset in several places. It replaces the ScopeMatcher with a SwitchOpcodeMatcher/SwitchType matcher by changing what the owner of ScopeMatcher is pointing to. This requires being able to modify the unique_ptr inside the owner.