[PATCH] D92739: [ValueTracking] Branch on poison is UB

Sun Jan 17 14:11:11 PST 2021

nikic added a comment.

I've reported the polly bug at https://bugs.llvm.org/show_bug.cgi?id=48783.

In D92739#2436359 <https://reviews.llvm.org/D92739#2436359>, @aqjune wrote:

> In D92739#2436145 <https://reviews.llvm.org/D92739#2436145>, @nikic wrote:
>
>> In D92739#2436141 <https://reviews.llvm.org/D92739#2436141>, @fhahn wrote:
>>
>>> I put up D80875 <https://reviews.llvm.org/D80875> a while ago which I think did something similar. Consensus at the time was that there still are some transforms that may introduce branches on undef/ poison and we should fix them before.
>>
>> I see... Unfortunately this is a bit of a chicken and egg problem. In this instance, my primary interest is to fix a poison handling bug in LSR, but if programUndefinedIfPoison() is artificially crippled, the fix has much more fallout than it should. Stronger poison logic allows us to fix poison bugs without unduly affecting optimization.
>
> This boils down to making miscompilation fixes cheap..
>
> Since the LSR bug is a real one, it has to be fixed without being blocked by those.
> I'm not a fan of this solution, but what about adding a parameter to programUndefinedIfPoison that allows the function to return true when br/switch is given, and let LSR call it with the flag set?

I believe the change to programUndefinedIfPoison primarily strengthens isGuaranteedNotToBePoison (which we want) and affects SCEV. For SCEV, the effect is that more nowrap flags can be transferred from IR to SCEV. We already transfer nowrap flags for expressions used in the latch condition, if the latch is the unique exiting block. This extends it to work on the first exiting condition in general (modulo various details). The transferal of flags from the latch condition already exploits branch on poison being UB, so this doesn't change much in terms of assumptions we make. (The code provides different reasoning for why it is correct for latches, but that reasoning is not actually correct in absence of mustprogress -- but it **is** correct when taking branch on poison UB into account.)

That is to say, I think this is one of the safer poison-related changes. I may have to go for the flag option to get the LSR bug fixed for LLVM 12 though.

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

https://reviews.llvm.org/D92739