[SSAUpdaterBulk] Add PHI simplification pass. #132004

vpykhtin · 2025-03-19T11:08:46Z

This patch, along with #131762, enhances SSAUpdaterBulk to function as a near drop-in replacement for SSAUpdater, see #130611.

I opted to implement phi optimization as a separate step to maintain the current behavior of SSAUpdaterBulk::RewriteAllUses while ensuring the implementation remains straightforward.

llvmbot · 2025-03-19T11:09:21Z

@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-llvm-transforms

Author: Valery Pykhtin (vpykhtin)

Changes

This patch, along with #131762, enhances SSAUpdaterBulk to function as a near drop-in replacement for SSAUpdater, see #130611.

I opted to implement phi optimization as a separate step to maintain the current behavior of SSAUpdaterBulk::RewriteAllUses while ensuring the implementation remains straightforward.

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

2 Files Affected:

(modified) llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h (+24)
(modified) llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp (+47)

diff --git a/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h b/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
index ad24cb454d5e7..6894e4fa0a357 100644
--- a/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
+++ b/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
@@ -79,6 +79,30 @@ class SSAUpdaterBulk {
   /// vector.
   void RewriteAllUses(DominatorTree *DT,
                       SmallVectorImpl<PHINode *> *InsertedPHIs = nullptr);
+
+  /// Rewrite all uses and simplify the inserted PHI nodes.
+  /// Use this method to preserve behavior when replacing SSAUpdater.
+  void RewriteAndSimplifyAllUses(DominatorTree *DT) {
+    SmallVector<PHINode *, 8> NewPHIs;
+    RewriteAllUses(DT, &NewPHIs);
+    simplifyPass(NewPHIs);
+  }
+
+  /// Same as previous, but return inserted PHI nodes in InsertedPHIs.
+  void RewriteAndSimplifyAllUses(DominatorTree *DT,
+                                 SmallVectorImpl<PHINode *> &InsertedPHIs) {
+    RewriteAllUses(DT, &InsertedPHIs);
+    simplifyPass(InsertedPHIs);
+    // Remove nullptrs from the worklist
+    InsertedPHIs.erase(
+        std::remove(InsertedPHIs.begin(), InsertedPHIs.end(), nullptr),
+        InsertedPHIs.end());
+  }
+
+private:
+  /// Perform a single pass of simplification over the worklist of PHIs.
+  /// Pointers to replaced phis are nulled out.
+  static bool simplifyPass(SmallVectorImpl<PHINode *> &Worklist);
 };
 
 } // end namespace llvm
diff --git a/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp b/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp
index cad7ff64c01fb..dac964b092ec5 100644
--- a/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp
+++ b/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/SSAUpdaterBulk.h"
+#include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/IteratedDominanceFrontier.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Dominators.h"
@@ -182,3 +183,49 @@ void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT,
     }
   }
 }
+
+static bool isEquivalentPHI(PHINode *PHI1, PHINode *PHI2) {
+  if (PHI1->getNumIncomingValues() != PHI2->getNumIncomingValues())
+    return false;
+
+  unsigned I = 0, NumValues = PHI1->getNumIncomingValues();
+  for (; I != NumValues; ++I) {
+    if (PHI1->getIncomingBlock(I) != PHI2->getIncomingBlock(I))
+      break;
+    if (PHI1->getIncomingValue(I) != PHI2->getIncomingValue(I))
+      return false;
+  }
+  // TODO: add procesing if phis have different order of incoming values.
+  return I == NumValues;
+}
+
+bool SSAUpdaterBulk::simplifyPass(SmallVectorImpl<PHINode *> &Worklist) {
+  if (Worklist.empty())
+    return false;
+
+  auto findEquivalentPHI = [](PHINode *PHI) -> Value * {
+    BasicBlock *BB = PHI->getParent();
+    for (auto &OtherPHI : BB->phis()) {
+      if (PHI != &OtherPHI && isEquivalentPHI(PHI, &OtherPHI)) {
+        return &OtherPHI;
+      }
+    }
+    return nullptr;
+  };
+
+  const DataLayout &DL = Worklist.front()->getParent()->getDataLayout();
+  bool Change = false;
+  for (PHINode *&PHI : Worklist) {
+    Value *Replacement = simplifyInstruction(PHI, DL);
+    if (!Replacement)
+      Replacement = findEquivalentPHI(PHI);
+
+    if (Replacement) {
+      PHI->replaceAllUsesWith(Replacement);
+      PHI->eraseFromParent();
+      PHI = nullptr; // Mark as removed
+      Change = true;
+    }
+  }
+  return Change;
+}

nikic · 2025-03-19T21:20:43Z

llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp

+      }
+    }
+    return nullptr;
+  };


Can we just use EliminateDuplicatePHINodes here?

The EliminateDuplicatePHINodes function has two main issues. First, it replaces later PHI nodes with earlier ones, leading to unexpected behavior where existing PHI nodes are replaced with new ones. Second, it tests all PHI nodes instead of focusing solely on the newly added ones, which would be more efficient.

nikic · 2025-03-19T21:21:37Z

I opted to implement phi optimization as a separate step to maintain the current behavior of SSAUpdaterBulk::RewriteAllUses while ensuring the implementation remains straightforward.

Do we have some uses that don't want the simplification behavior? If not, I'd rather change the behavior of the existing methods.

vpykhtin · 2025-03-20T05:32:46Z

Do we have some uses that don't want the simplification behavior? If not, I'd rather change the behavior of the existing methods.

I don't know, it's very likely that everybody would prefer optimizing version.

In this case it would be easier to add EliminateDuplicatePHINodes to the end of RewriteAllUses as I need to collect the set of BB to process.

vpykhtin · 2025-03-20T10:48:17Z

The EliminateDuplicatePHINodes function has two main issues. First, it replaces later PHI nodes with earlier ones, leading to unexpected behavior where existing PHI nodes are replaced with new ones. Second, it tests all PHI nodes instead of focusing solely on the newly added ones, which would be more efficient.

github-actions · 2025-03-21T10:25:01Z

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

vpykhtin · 2025-03-21T10:25:17Z

I peeked into EliminateDuplicatePHINodes and used isIdenticalToWhenDefined instead of new code.

vpykhtin · 2025-03-24T07:46:30Z

Hi @nikic, I believe this is ready for submission in its current state. I chose to keep this code as a separate 'pass' rather than integrating it directly into RewriteAllUses for two reasons:

Identifying an equivalent PHI in RewriteAllUses and performing the subsequent RAUW would require using TrackingVH in BBInfo.
We can rename RewriteAndSimplifyAllUses to RewriteAllUses in a separate patch.

vpykhtin · 2025-03-25T12:27:13Z

Used EliminateDuplicatePHINodes because it's much faster.

Turned on optimizations by default, got some failures but they're mostly reorder of phi's.

vpykhtin · 2025-03-31T12:29:33Z

Current implementation is incorrect: I cannot move new phis to the end because they can be used by existing phis if the user added 'variable' as incoming value for an existed phi.

nikic · 2025-04-04T09:02:28Z

Current implementation is incorrect: I cannot move new phis to the end because they can be used by existing phis if the user added 'variable' as incoming value for an existed phi.

Maybe I'm misunderstanding what you mean here, but phi's are all evaluated at the same time, not in order. If a phi refers to another phi, it always refers to the value of the phi at a previous cycle iteration.

vpykhtin · 2025-04-06T12:08:58Z

Maybe I'm misunderstanding what you mean here, but phi's are all evaluated at the same time, not in order. If a phi refers to another phi, it always refers to the value of the phi at a previous cycle iteration.

Sorry, false alarm, otherwise phis wouldn't be able to reference itself. I'm preparing new patch as a stacked graphite PR that would include this patch and its' use in AMDGPU backend.

nikic · 2025-04-06T19:27:32Z

llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp

+    return false;
+
+  const DataLayout &DL = Worklist.front()->getParent()->getDataLayout();
+  bool Change = false;


Suggested change

bool Change = false;

bool Changed = false;

nit: More typical naming for this flag.

Will address this in the new stacker PR.

llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp

nikic · 2025-04-06T19:32:10Z

llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp

+    if (Value *Replacement = simplifyInstruction(PHI, DL)) {
+      PHI->replaceAllUsesWith(Replacement);
+      PHI->eraseFromParent();
+      PHI = nullptr; // Mark as removed


You could also iterate over Worklist using erase_if instead to avoid leaving behind nullptrs.

Agree, but I don't see much sense since this is internal routine and we're know what we're doing after that :)

llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp

vpykhtin · 2025-04-10T14:11:40Z

Hi @nikic, sorry for the mess, I've replaced this PR with new stacked version, #135180, I had to start it someday.

[SSAUpdaterBulk] Add PHI simplification pass.

8596961

llvmbot added the llvm:transforms label Mar 19, 2025

vpykhtin requested a review from arsenm March 19, 2025 11:09

add test.

10e99d6

vpykhtin requested a review from nikic March 19, 2025 17:52

nikic reviewed Mar 19, 2025

View reviewed changes

vpykhtin added 2 commits March 21, 2025 09:51

Merge branch 'main' into ssaupdaterbulk_simp_phis

09f1e89

Use isIdenticalToWhenDefined

241b300

formatting

fba85b4

vpykhtin mentioned this pull request Mar 23, 2025

[AMDGPU] Improve StructurizeCFG pass performance by using SSAUpdaterBulk. #130611

Closed

Use EliminateDuplicatePHINodes. Make optimizations by default.

6264060

vpykhtin requested a review from nikic March 25, 2025 16:50

Update tests.

23f244e

llvmbot added the backend:WebAssembly label Mar 26, 2025

vpykhtin requested review from aheejin, UsmanNadeem and last5bits March 26, 2025 06:50

nikic reviewed Apr 6, 2025

View reviewed changes

vpykhtin mentioned this pull request Apr 10, 2025

[SSAUpdaterBulk] Add PHI simplification pass. #135180

Closed

vpykhtin closed this Apr 10, 2025

vpykhtin mentioned this pull request Jul 28, 2025

[SSAUpdaterBulk] Add PHI simplification pass. #150936

Open

[SSAUpdaterBulk] Add PHI simplification pass. #132004

[SSAUpdaterBulk] Add PHI simplification pass. #132004

Uh oh!

Conversation

vpykhtin commented Mar 19, 2025

Uh oh!

llvmbot commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

vpykhtin Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

nikic commented Mar 19, 2025

Uh oh!

vpykhtin commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpykhtin commented Mar 20, 2025

Uh oh!

github-actions bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpykhtin commented Mar 21, 2025

Uh oh!

vpykhtin commented Mar 24, 2025

Uh oh!

vpykhtin commented Mar 25, 2025

Uh oh!

vpykhtin commented Mar 31, 2025

Uh oh!

nikic commented Apr 4, 2025

Uh oh!

vpykhtin commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic Apr 6, 2025

Choose a reason for hiding this comment

Uh oh!

vpykhtin Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic Apr 6, 2025

Choose a reason for hiding this comment

Uh oh!

vpykhtin Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vpykhtin commented Apr 10, 2025

Uh oh!

Uh oh!

llvmbot commented Mar 19, 2025 •

edited

Loading

vpykhtin commented Mar 20, 2025 •

edited

Loading

github-actions bot commented Mar 21, 2025 •

edited

Loading

vpykhtin commented Apr 6, 2025 •

edited

Loading