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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:97
 STATISTIC(NumPRELoopLoad, "Number of loop loads PRE'd");
+STATISTIC(NumPRELoadMoved, "Number of moved loads in PRE");
 
----------------
More specific name, smth related to critical edges maybe?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1640
+    if (NumInsertPreds > 1)
+      return false;
   }
----------------
Is it really needed? There is literally same check just below.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2703
     for (auto *I : InstrsToErase) {
-      assert(I->getParent() == BB && "Removing instruction from wrong block?");
       LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
----------------
I think this assert should not fail, and if it fails, you have a bug. `ICF` may keep cached information for it bound to its old parent, and will try to remove it from its new parent. You may get inconsistent state of `ICF` because of it. At least I don't see how you update it.

You should not move instructions. The right approach is to create a new one.


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

https://reviews.llvm.org/D141712