[PATCH] D88725: [GVN LoadPRE] Extend the scope of optimization by using context to prove safety of speculation

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/minor fixes.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1170
     // Check that there is no implicit control flow in a block above.
-    if (!IsSafeToSpeculativelyExecute && ICF->hasICF(TmpBB))
-      return false;
+    NeedSafeToSpeculativelyExecute =
+        NeedSafeToSpeculativelyExecute || ICF->hasICF(TmpBB);
----------------
Use "|=" please.


================
Comment at: llvm/test/Transforms/GVN/loadpre-context.ll:4
 
 ; load may be speculated, adress is not null using context search.
 ; There is a critical edge.
----------------
"address" not "adress".  Consistent spelling mistake in file, please fix.  


================
Comment at: llvm/test/Transforms/GVN/loadpre-context.ll:96
 
 ; load cannot be speculated, adress is not null check does not dominate the loop.
 define i32 @loadpre_maybe_null(i32* align 8 dereferenceable_or_null(48) %arg, i32 %N, i1 %c) {
----------------
I think your comment is missing some punctuation.  This reads very oddly, reword please.


================
Comment at: llvm/test/Transforms/GVN/loadpre-context.ll:148
 ; Does not guarantee that returns.
 declare i32 @foo(i32) readnone
----------------
Please rename this to something like ro_foo.  i.e. make the name explicit about being read only.  

Alternatively, use the readnone attribute on the call instruction, not the decl.  

Otherwise, you confuse the reader because it looks like you're hoisting a load past a mutating call.  I had a comment all written before thinking to check the declaration.  :)


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

https://reviews.llvm.org/D88725