[PATCH] D103316: Hoist llvm.assume into single predecessor if block otherwise empty

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:380
 
     void removeAllAssertingVHReferences(Value *V);
-    bool eliminateAssumptions(Function &F);
----------------
Please split the new transform in SimplifyCFG and the deletion of the old CGP code into two patches.  The former should be independently worthwhile if structured properly.  


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6817
 
+  // Hoist @llvm.assumes if they (and their inputs) are the only contents of
+  // the block.
----------------
I don't think this is the right framing.  We don't want to unconditionally hoist assumes, we only want to hoist them if they'd otherwise inhibit a transformation.  (e.g. If the successor block was otherwise empty, we hoist as part of threading the edge to the successors successor.)


================
Comment at: llvm/test/Transforms/InstCombine/assume-align.ll:16
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP4]])
+; CHECK-NEXT:    br i1 [[TMP2]], label [[IF_THEN1:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then1:
----------------
If we don't predicate the store, the shown transform here does not appear profitable.  


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

https://reviews.llvm.org/D103316