[GVN] Improve processBlock for instruction erasure #131753

madhur13490 · 2025-03-18T08:12:52Z

This patch deletes the instructions right away by using the approapriate iterators. Thus avoids collecting the instructions in a vector and then doing the erasure.

llvmbot · 2025-03-18T08:13:26Z

@llvm/pr-subscribers-llvm-transforms

Author: Madhur Amilkanthwar (madhur13490)

Changes

This patch deletes the instructions right away by using the approapriate iterators. Thus avoids collecting the instructions in a vector and then doing the erasure.

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

2 Files Affected:

(modified) llvm/include/llvm/Transforms/Scalar/GVN.h (+6-3)
(modified) llvm/lib/Transforms/Scalar/GVN.cpp (+15-47)

diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h
index c8be390799836..01b3763fb5581 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVN.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVN.h
@@ -25,6 +25,8 @@
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Transforms/Utils/AssumeBundleBuilder.h"
+#include "llvm/Transforms/Utils/Local.h"
 #include <cstdint>
 #include <optional>
 #include <utility>
@@ -137,9 +139,11 @@ class GVNPass : public PassInfoMixin<GVNPass> {
 
   /// This removes the specified instruction from
   /// our various maps and marks it for deletion.
-  void markInstructionForDeletion(Instruction *I) {
+  void doInstructionDeletion(Instruction *I) {
+    salvageKnowledge(I, AC);
+    salvageDebugInfo(*I);
     VN.erase(I);
-    InstrsToErase.push_back(I);
+    removeInstruction(I);
   }
 
   DominatorTree &getDominatorTree() const { return *DT; }
@@ -306,7 +310,6 @@ class GVNPass : public PassInfoMixin<GVNPass> {
   // propagate to any successors. Entries added mid-block are applied
   // to the remaining instructions in the block.
   SmallMapVector<Value *, Value *, 4> ReplaceOperandsWithMap;
-  SmallVector<Instruction *, 8> InstrsToErase;
 
   // Map the block to reversed postorder traversal number. It is used to
   // find back edge easily.
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 85fcdd95f5d87..5550721f8d249 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -730,8 +730,9 @@ void GVNPass::ValueTable::erase(Value *V) {
 /// verifyRemoved - Verify that the value is removed from all internal data
 /// structures.
 void GVNPass::ValueTable::verifyRemoved(const Value *V) const {
-  assert(!valueNumbering.contains(V) &&
-         "Inst still occurs in value numbering map!");
+  if (V != nullptr)
+    assert(!valueNumbering.contains(V) &&
+           "Inst still occurs in value numbering map!");
 }
 
 //===----------------------------------------------------------------------===//
@@ -1574,7 +1575,7 @@ void GVNPass::eliminatePartiallyRedundantLoad(
     I->setDebugLoc(Load->getDebugLoc());
   if (V->getType()->isPtrOrPtrVectorTy())
     MD->invalidateCachedPointerInfo(V);
-  markInstructionForDeletion(Load);
+  doInstructionDeletion(Load);
   ORE->emit([&]() {
     return OptimizationRemark(DEBUG_TYPE, "LoadPRE", Load)
            << "load eliminated by PRE";
@@ -1798,7 +1799,7 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
       // Erase instructions generated by the failed PHI translation before
       // trying to number them. PHI translation might insert instructions
       // in basic blocks other than the current one, and we delete them
-      // directly, as markInstructionForDeletion only allows removing from the
+      // directly, as doInstructionDeletion only allows removing from the
       // current basic block.
       NewInsts.pop_back_val()->eraseFromParent();
     }
@@ -1997,7 +1998,7 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
         I->setDebugLoc(Load->getDebugLoc());
     if (V->getType()->isPtrOrPtrVectorTy())
       MD->invalidateCachedPointerInfo(V);
-    markInstructionForDeletion(Load);
+    doInstructionDeletion(Load);
     ++NumGVNLoad;
     reportLoadElim(Load, V, ORE);
     return true;
@@ -2067,7 +2068,7 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) {
       }
     }
     if (isAssumeWithEmptyBundle(*IntrinsicI)) {
-      markInstructionForDeletion(IntrinsicI);
+      doInstructionDeletion(IntrinsicI);
       return true;
     }
     return false;
@@ -2179,7 +2180,7 @@ bool GVNPass::processLoad(LoadInst *L) {
     return false;
 
   if (L->use_empty()) {
-    markInstructionForDeletion(L);
+    doInstructionDeletion(L);
     return true;
   }
 
@@ -2209,13 +2210,13 @@ bool GVNPass::processLoad(LoadInst *L) {
   // MaterializeAdjustedValue is responsible for combining metadata.
   ICF->removeUsersOf(L);
   L->replaceAllUsesWith(AvailableValue);
-  markInstructionForDeletion(L);
   if (MSSAU)
     MSSAU->removeMemoryAccess(L);
   ++NumGVNLoad;
   reportLoadElim(L, AvailableValue, ORE);
   // Tell MDA to reexamine the reused pointer since we might have more
   // information after forwarding it.
+  doInstructionDeletion(L);
   if (MD && AvailableValue->getType()->isPtrOrPtrVectorTy())
     MD->invalidateCachedPointerInfo(AvailableValue);
   return true;
@@ -2605,7 +2606,7 @@ bool GVNPass::processInstruction(Instruction *I) {
       Changed = true;
     }
     if (isInstructionTriviallyDead(I, TLI)) {
-      markInstructionForDeletion(I);
+      doInstructionDeletion(I);
       Changed = true;
     }
     if (Changed) {
@@ -2723,7 +2724,7 @@ bool GVNPass::processInstruction(Instruction *I) {
   patchAndReplaceAllUsesWith(I, Repl);
   if (MD && Repl->getType()->isPtrOrPtrVectorTy())
     MD->invalidateCachedPointerInfo(Repl);
-  markInstructionForDeletion(I);
+  doInstructionDeletion(I);
   return true;
 }
 
@@ -2799,10 +2800,6 @@ bool GVNPass::runImpl(Function &F, AssumptionCache &RunAC, DominatorTree &RunDT,
 }
 
 bool GVNPass::processBlock(BasicBlock *BB) {
-  // FIXME: Kill off InstrsToErase by doing erasing eagerly in a helper function
-  // (and incrementing BI before processing an instruction).
-  assert(InstrsToErase.empty() &&
-         "We expect InstrsToErase to be empty across iterations");
   if (DeadBlocks.count(BB))
     return false;
 
@@ -2820,41 +2817,12 @@ bool GVNPass::processBlock(BasicBlock *BB) {
     VN.erase(PN);
     removeInstruction(PN);
   }
-
-  for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
-       BI != BE;) {
+  for (BasicBlock::iterator BI = BB->begin(), BE = BB->end(); BI != BE;) {
+    Instruction *Inst = &*BI++;
     if (!ReplaceOperandsWithMap.empty())
-      ChangedFunction |= replaceOperandsForInBlockEquality(&*BI);
-    ChangedFunction |= processInstruction(&*BI);
-
-    if (InstrsToErase.empty()) {
-      ++BI;
-      continue;
-    }
-
-    // If we need some instructions deleted, do it now.
-    NumGVNInstr += InstrsToErase.size();
-
-    // Avoid iterator invalidation.
-    bool AtStart = BI == BB->begin();
-    if (!AtStart)
-      --BI;
-
-    for (auto *I : InstrsToErase) {
-      assert(I->getParent() == BB && "Removing instruction from wrong block?");
-      LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
-      salvageKnowledge(I, AC);
-      salvageDebugInfo(*I);
-      removeInstruction(I);
-    }
-    InstrsToErase.clear();
-
-    if (AtStart)
-      BI = BB->begin();
-    else
-      ++BI;
+      ChangedFunction |= replaceOperandsForInBlockEquality(Inst);
+    ChangedFunction |= processInstruction(Inst);
   }
-
   return ChangedFunction;
 }

madhur13490 · 2025-03-18T08:14:00Z

All tests pass but not sure how to construct a test for this patch. Suggestions welcome!

nikic

Could you provide more details on the motivation for this change? Is it just to resolve the FIXME or something else?

It looks like the stack-layout-remark.c clang test crashes.

madhur13490 · 2025-03-18T15:11:05Z

Could you provide more details on the motivation for this change? Is it just to resolve the FIXME or something else?

yes, primarily FIXME. Nothing else as such which motivates. The change is an optimization in the process of handling instructions by avoiding vector population just for the erasure.

It looks like the stack-layout-remark.c clang test crashes.

Fixed in the current revision. Thanks!

madhur13490 · 2025-04-02T03:05:48Z

@nikic Gentle ping!

madhur13490 · 2025-04-10T17:30:45Z

Ping to reviewers! @antoniofrighetto can you please have a look?

llvm/lib/Transforms/Scalar/GVN.cpp

antoniofrighetto · 2025-04-10T18:35:38Z

llvm/include/llvm/Transforms/Scalar/GVN.h

@@ -25,6 +25,8 @@
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Transforms/Utils/AssumeBundleBuilder.h"


Can remove this from the C++ file?

IMHO it would be better to move the definition of doInstructionDeletion to the source file. There's no need to have it in the header.

Can drop these includes now.

Can't remove. salvage* is provided by AssumeBundleBuilder.

Yes, but it's no longer necessary in the header. You can drop the includes in the header and restore the AssumeBundleBuilder.h include in the cpp file.

You can drop the includes in the header

Do you mean just the AssumeBundleBuilder?

I mean the two you added.

nikic · 2025-04-11T12:55:01Z

llvm/include/llvm/Transforms/Scalar/GVN.h

@@ -25,6 +25,8 @@
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Transforms/Utils/AssumeBundleBuilder.h"


IMHO it would be better to move the definition of doInstructionDeletion to the source file. There's no need to have it in the header.

llvm/include/llvm/Transforms/Scalar/GVN.h

llvm/lib/Transforms/Scalar/GVN.cpp

github-actions · 2025-04-21T04:54:18Z

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

madhur13490 · 2025-04-21T04:58:57Z

@antoniofrighetto @nikic Addressed all review comments. The thing about guarding assert under V != nullptr in verifyRemoved() was added for some test but I don't remember which one. Running ninja check-all today leads to no new failures, thus I have removed that change. Probably the issue is fixed somewhere else.

madhur13490 · 2025-04-28T06:06:38Z

Gentle ping @antoniofrighetto @nikic !

nikic · 2025-04-28T20:49:15Z

llvm/include/llvm/Transforms/Scalar/GVN.h

@@ -25,6 +25,8 @@
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Transforms/Utils/AssumeBundleBuilder.h"


Can drop these includes now.

llvm/lib/Transforms/Scalar/GVN.cpp

This patch deletes the instructions right away by using the approapriate iterators. Thus avoids collecting the instructions in a vector and then doing the erasure.

madhur13490 · 2025-04-29T09:00:49Z

@nikic Removed the new includes from GVN.h and headers in GVN.cpp are untouched now.

nikic

LGTM

This patch deletes the instructions immediately in core GVN processing by using the appropriate iterators. Thus, it avoids collecting the instructions in a vector and then doing the erasure.

madhur13490 requested review from alinas and nikic March 18, 2025 08:12

llvmbot added the llvm:transforms label Mar 18, 2025

madhur13490 requested a review from weiguozhi March 18, 2025 08:13

nikic reviewed Mar 18, 2025

View reviewed changes

madhur13490 force-pushed the elim_erase branch from 4de1a9e to 9f21c0d Compare March 18, 2025 15:08

madhur13490 requested a review from antoniofrighetto April 10, 2025 17:30

antoniofrighetto reviewed Apr 10, 2025

View reviewed changes

madhur13490 force-pushed the elim_erase branch from 9f21c0d to 6d67d45 Compare April 11, 2025 08:32

nikic reviewed Apr 11, 2025

View reviewed changes

madhur13490 force-pushed the elim_erase branch from 6d67d45 to 9a437c7 Compare April 21, 2025 04:52

madhur13490 force-pushed the elim_erase branch from 9a437c7 to 170c8f8 Compare April 21, 2025 04:55

nikic reviewed Apr 28, 2025

View reviewed changes

madhur13490 added 2 commits April 28, 2025 23:32

[GVN] Improve processBlock for instruction erasure

a54e0a5

This patch deletes the instructions right away by using the approapriate iterators. Thus avoids collecting the instructions in a vector and then doing the erasure.

Address review comments

7b358ed

madhur13490 force-pushed the elim_erase branch from 170c8f8 to 738735b Compare April 29, 2025 07:58

Address review comments

9ad0eaf

madhur13490 force-pushed the elim_erase branch from 738735b to 9ad0eaf Compare April 29, 2025 08:59

nikic approved these changes Apr 29, 2025

View reviewed changes

madhur13490 merged commit 015093d into llvm:main May 6, 2025
11 checks passed

[GVN] Improve processBlock for instruction erasure #131753

[GVN] Improve processBlock for instruction erasure #131753

Uh oh!

Conversation

madhur13490 commented Mar 18, 2025

Uh oh!

llvmbot commented Mar 18, 2025

Uh oh!

madhur13490 commented Mar 18, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

madhur13490 commented Apr 2, 2025

Uh oh!

madhur13490 commented Apr 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

madhur13490 commented Apr 21, 2025

Uh oh!

madhur13490 commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

madhur13490 commented Apr 29, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

madhur13490 commented Mar 18, 2025 •

edited

Loading

github-actions bot commented Apr 21, 2025 •

edited

Loading