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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.

Looks close to ready for an LGTM if you're willing to split patch as suggested.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1496
+
+    // Make sure that the terminator itself doesn't clobber.
+    if (Blocker->getTerminator()->mayWriteToMemory())
----------------
continue the comment with something like:
"because we need a place to insert a copy of the load".

p.s. I'm fine with this in an initial patch, but you really should be using an alias check here as the trailing invoke might not alias the memory being PREed.  Would make a good follow up patch.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1515
+  bool ProvedNoDealloc =
+      isa<AllocaInst>(LoadPtr) || isSafeToSpeculativelyExecute(Load) ||
+      all_of(*LoopBlock, [](Instruction &I) {
----------------
You can generalize the first check as !LoadPtr->canBeFreed()


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1516
+      isa<AllocaInst>(LoadPtr) || isSafeToSpeculativelyExecute(Load) ||
+      all_of(*LoopBlock, [](Instruction &I) {
+        if (auto *CI = dyn_cast<CallInst>(&I))
----------------
This last check is incorrect.  Counter example:
for (i = 0; i < 1; i++)
  v = o.f
  if (c) {
    // this is loop block
    clobber();
  }
  atomic store g = o;
  while(wait for other thread to free) {}
}

Can I ask you to pull this into a separate patch?  (e.g. handle only the first two cases in this patch, and come back to the third in a follow on.)


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1519
+          return !CI->mayWriteToMemory() ||
+                 CI->getAttributes().hasAttribute(AttributeList::FunctionIndex,
+                                                  Attribute::NoFree);
----------------
Style: hasFnAttribute(AttributeKind::NoFree) handles both of these cases.  (Or will once D100226 lands).


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

https://reviews.llvm.org/D99926