[PATCH] D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors

[Philip Reames via Phabricator via llvm-commits]

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

- List Item

The loadPRE part of this seems very close to being ready to submit.



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2189
 
+  // Make sure that there are no instructions with implicit control flow that
+  // could prevent us from reaching our instruction.
----------------
This is overly conservative for any instruction which can not ever fault.  For instance, PRE a load over a throwing call is perfectly sound.  Note that preserving the nsw/nuw flags is questionable in the same case, but that's a hard discussion.  :)

I'd prefer to see the scalarPRE part separate into it's own patch.  I know I'm saying the exact opposite of Eli here, but I think this needs more discussion and trying to combine them will delay the loadPRE patch unnecessarily.  


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2406
+      if (!isGuaranteedToTransferExecutionToSuccessor(&I) &&
+          !isa<LoadInst>(&I) && !isa<StoreInst>(&I)) {
+        FirstImplicitControlFlowInsts[BB] = &I;
----------------
This is scarily permissive.  It (for instance) allows ordered loads and stores which (I think) is valid.  It'd be much safer to add a special matcher for volatile-only loads/stores here.


https://reviews.llvm.org/D37460