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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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

> In D103316#2786946 <https://reviews.llvm.org/D103316#2786946>, @nikic wrote:
>
>>> This kind of transformation should probably eventually reside in lib/Transforms/Utils/SimplifyCFG.cpp but for now it seemed easier and a smaller change to just leave it in CodeGenPrepare.
>>
>> I don't think this transform makes a lot of sense in CGP, I'd prefer it to go directly to SimplifyCFG (after which we can drop the CGP code).
>
> I agree.
>
>> I think we would be better off to just drop these assumes. The framing I'm thinking of here is that SimplifyCFG should generally be ignoring ephemeral values in transforms. So if normally SimplifyCFG would drop a block that contains no instructions, then it should also drop a block that only contains ephemeral values.
>
> I disagree.
>
>   if (x)
>     assume(y)
>
> is a natural way to spell something for the user.
> We should not just drop it without having a good reason.
> While we might not use assume(x && y) directly, we will use it if we specialize x to true in some places.

Er, I think you misread the comment you were replying to.  The comment was discussing assume(A || B), your response is discussing assume(A && B).  Given the context of the discussion, that difference seems important.

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.


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

https://reviews.llvm.org/D103316