[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:1516
+      isa<AllocaInst>(LoadPtr) || isSafeToSpeculativelyExecute(Load) ||
+      all_of(*LoopBlock, [](Instruction &I) {
+        if (auto *CI = dyn_cast<CallInst>(&I))
----------------
reames wrote:
> 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.)
Good idea. Removed from this patch, need to make it more carefully.


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


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

https://reviews.llvm.org/D99926