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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Comments inline include one serious correctness issue.

This is much cleaner than the original patch.  I was initially hesitant to take this at all - as opposed to using MemorySSA or NewGVN - but with the new structure this looks a lot less invasive.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1467
+
+  // We can side-exit before the load is executed.
+  if (ICF->isDominatedByICFIFromSameBlock(Load))
----------------
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. 


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1481
+    UnavailableLoopBlocks++;
+    // So far, only PRE into not-mustexecute blocks.
+    if (DT->dominates(Blocker, Latch)) {
----------------
Tweak this comment a bit to emphasize that this ensures the new load executes at most as often as the original, and likely less often.  


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1486
+
+    if (!isa<BranchInst>(Blocker->getTerminator()))
+      return false;
----------------
I don't understand this restriction.  Why is a switch not allowed?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1497
+  MapVector<BasicBlock *, Value *> AvailableLoads;
+  for (auto *Blocker : UnavailableBlocks)
+    AvailableLoads[Blocker] = LoadPtr;
----------------
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.


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

https://reviews.llvm.org/D99926