[PATCH] D99926: [GVN] Introduce loop load PRE

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1467
+
+  // We can side-exit before the load is executed.
+  if (ICF->isDominatedByICFIFromSameBlock(Load))
----------------
reames wrote:
> Extend this comment to emphasize that this means we have proven the load must execute if the loop is entered, and is thus safe to hoist to the end of the preheader without introducing a new fault.  Similarly, the in-loop clobber must be dominated by the original load and is thus fault safe.
> 
> Er, hm, there's an issue here I just realized.  This isn't sound.  The counter example here is when the clobber is a call to free and the loop actually runs one iteration.
> 
> You need to prove that LI is safe to execute in both locations.  You have multiple options in terms of reasoning, I'll let you decide which you want to explore: speculation safety, must execute, or unfreeable allocations.  The last (using allocas as an example for test), might be the easiest. 
Free on the last iteration (the loop may have multiple though) is a nasty case indeed...


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1486
+
+    if (!isa<BranchInst>(Blocker->getTerminator()))
+      return false;
----------------
reames wrote:
> I don't understand this restriction.  Why is a switch not allowed?
This was ogirinally the protection against invokes. Switch is allowed, will fix.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1497
+  MapVector<BasicBlock *, Value *> AvailableLoads;
+  for (auto *Blocker : UnavailableBlocks)
+    AvailableLoads[Blocker] = LoadPtr;
----------------
reames wrote:
> I don't think this loop does what you want, except maybe by accident.  You allowed blocks outside the loop, as a result, you can end up with a bunch of available addresses and a bunch of loads before the preheader.  This will likely later be DCEd since the preheader load will be the one actually used by SSA gen.
> 
> I strongly suspect you want exactly two available load locations: preheader, and your one in-loop clobber block.
Yes, this check was lost during the refactoring. I'm pretty sure that `eliminatePartiallyRedundantLoad` will deal with it correctly, but it's at least not obvious. Thanks for catching.


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

https://reviews.llvm.org/D99926