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

[Markus Lavin via Phabricator via llvm-commits]

markus added a comment.

In D103316#2800153 <https://reviews.llvm.org/D103316#2800153>, @jdoerfert wrote:

> In D103316#2800110 <https://reviews.llvm.org/D103316#2800110>, @reames wrote:
>
>> My own opinion is closer to that of @nikic.  I think we should not be letting assumes block transforms.  We should attempt to salvage assume information where we can, but only on a best effort basis.  I do think we should generate the assume(A || B) form if that's the logical salvage even if nothing currently can infer from that.
>
> Unsure if I got it right or wrong before but what you said last is what I like too. Salvage assumes in blocks we want to remove if possible. At the same time, "ignore" assumes when the decision for transformations are made.

So what does this mean in practice? 
I interpret it as follows:
We provide a function say `bool isBlockEmptyExceptAssumes(BasicBlock *BB)` and we update the decision making of all the existing CFG transformations to use that. We provide another function say `void hoistAssumesFromBlockKnownToBeOtherwiseEmpty(BasicBlock *BB)` that all those CFG transformations need to call during their transformation if they used that fact that any of the blocks checked out as true with the former function.

Now maybe there aren't that many CFG transformations where this is relevant and maybe it integrates easily, I don't know, but it does seem like a somewhat larger change.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:380
 
     void removeAllAssertingVHReferences(Value *V);
-    bool eliminateAssumptions(Function &F);
----------------
reames wrote:
> 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.  
That makes sense. Will do.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6817
 
+  // Hoist @llvm.assumes if they (and their inputs) are the only contents of
+  // the block.
----------------
reames wrote:
> 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.)
I don't fully understand this comment. The current patch does not unconditionally hoist assumes. It hoists assumes if the block in question is otherwise empty. Thus preventing those assumes from inhibiting other transformations later on. Of course it will require another iteration of CFG considerations but isn't that the way these CFG optimizations are generally structured (one transform enables another transform)? 

I guess one way I could interpret this comment is that we should provide a utility function say `isBlockEmptyExceptAssumes()` that can then be queried in the decision making of all the other CFG transformations. So if a block only contains assumes then treat it as if it was empty when performing those optimizations. Maybe that is doable but it does not seem obvious that it would integrate easily with the existing transformations.


================
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:
----------------
reames wrote:
> If we don't predicate the store, the shown transform here does not appear profitable.  
I didn't even know that we had predicated scalar stores on IR. Either way is it a generic comment that it would be good if the stores was eventually predicated (in the final emission) or does it imply that predication should happen as part of `simplify-cfg` now with the added transformation?


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

https://reviews.llvm.org/D103316