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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D93065#2466356 <https://reviews.llvm.org/D93065#2466356>, @aqjune wrote:

> In D93065#2464486 <https://reviews.llvm.org/D93065#2464486>, @aqjune wrote:
>
>> Assuming that all relevant optimizations are fully implemented in both scenarios (and/or with freeze vs. select), I think their performance diff should be zero because C/C++ constrains variables to have well-defined values.
>> Currently, this isn't enforced by clang. We already have !noundef, and simply attaching it to loads will do the trick.
>
> Thinking about this again, this isn't true. Since !noundef can be stripped when the load is hoisted, there still is a possible UB loss.
> So, in terms of preserving UB, using select will be the better approach.
>
> But, there are so many transformations to consider if select is used; from InstCombine optimizations that involve `And`/`Or` to analysis, simplifycfg, etc. It seems to fix all of them is a hard job.

I think we'll just have to bite the bullet and do that. In a systematic way, before this lands. I think that the number of places that need to be changed is actually rather limited as we're only interested in and/or on booleans. We don't have //that// many analyses dealing with conditions.

Here's a sample patch for LVI: https://gist.github.com/nikic/fac8124a95901b7ff9ac44513f554578 With this PatternMatch boilerplate in place, the actual LVI change is trivial. The main work is adding relevant tests...

Worth noting is that some of the code deleted in this patch should be retained as a canonicalization. E.g. `a ? a : b` should become `a ? true : b`, `a ? b : true` should become `!a ? true : b`, etc. That way the remaining code only has to deal with canonical select patterns.

It may also be worthwhile to convert the special cases of `icmp (a, X) ? (icmp a, Y) : false` to `icmp (a, X) & (icmp a, Y)`, though I'm not sure if that is even useful if we handle the select pattern reasonable everywhere else.


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

https://reviews.llvm.org/D93065