[PATCH] D93065: [InstCombine] Disable optimizations of select instructions that causes propagation of poison values

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D93065#2463069 <https://reviews.llvm.org/D93065#2463069>, @nlopes wrote:

> In D93065#2461517 <https://reviews.llvm.org/D93065#2461517>, @congzhe wrote:
>
>> In D93065#2461196 <https://reviews.llvm.org/D93065#2461196>, @nlopes wrote:
>>
>>> Using freeze loses information (if some of the inputs was poison). Plus It requires an extra op.
>>> If we canonicalize around select there's no loss of information and it's just 1 instruction.
>>>
>>> The disadvantage is that then we have 2 ways or doing boolean ANDs/ORs. Though most analyses can be patched easily, as most LLVM analyses' results are of the form "x has property foo unless it's poison". So for those analyses using and/or or select is the same (as the only difference between these is propagation of poison).
>>> Other analyses/optimization can learn about select as needed.
>>
>> Thank you for raising up the good point! I understand that we lose information by preventing poison values from propagation using freeze. But I'm unclear what would be the side effect or problem with that? I'd appreciate it if you could clarify a bit, thanks!
>
> In practice, probably not a lot. But it may have implications for loop optimization, like:
>
>   for (i=0; some_bool && i < limit; ++i) {
>   ...
>   }
>
> If you remove the poison from the `i+1 < limit` bit it may make the work of SCEV harder (or impossible; didn't think the example through carefully).

Can I just make sure my understanding is correct -- so when we check the SCEV of `some_bool && i < limit;`  we do recursion backwards on this select instruction (after this patch) or on an AND instruction (before this patch). If we choose the freeze approach, we'll do recursion on the AND instruction and eventually hit a freeze instruction which SCEV does not know how to handle, hence SCEV will just return `CouldNotCompute`?


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

https://reviews.llvm.org/D93065