[PATCH] D139582: [GVN] Improve PRE on load instructions

Guozhi Wei via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 21 11:02:20 PST 2022


Carrot added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1397
+      // be safely moved to PredBB.
+      if (Dep.isNonLocal())
+        return static_cast<LoadInst *>(&Inst);
----------------
nikic wrote:
> Do we need to be concerned about speculating the load? You do perform a speculation check, but I think it does not cover the speculation of this load.
Do you mean the following code in function PerformLoadPRE ?
```
+    for (auto &CEP : CriticalEdgePredAndLoad)
+      if (!isSafeToSpeculativelyExecute(Load, CEP.first->getTerminator(), AC,
+                                        DT))
+        return false;
```

It's specifically for this purpose. I used the original Load instruction instead of CEP.second because all of them are identical.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2702
     for (auto *I : InstrsToErase) {
-      assert(I->getParent() == BB && "Removing instruction from wrong block?");
       LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
----------------
nikic wrote:
> I am not sure this assertion is safe to remove. I think a problem with your current code is that it may try to remove a load that is part of the leader table for that block, if that block has been processed before the current one.
Added code into function eliminatePartiallyRedundantLoad to delete the corresponding leader table entry.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139582/new/

https://reviews.llvm.org/D139582



More information about the llvm-commits mailing list