[PATCH] D99674: [InstCombine] Conditionally fold select i1 into and/or

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D99674#2662023 <https://reviews.llvm.org/D99674#2662023>, @nikic wrote:

>> Unconditionally disabling this transformation affects later pipelines that depend on and/or i1s.
>> To minimize its impact, this patch conservatively checks whether cond2 is an instruction that
>> creates a poison or its operand creates a poison.
>
> Do you have any further information on the regressions this would result in?
>
> Given all the preliminary work that has gone into this, I'd really like to have the option flipped outright, and not introduce another hack.

If `-instcombine-unsafe-select-transform` is flipped from true to false, we have 35 LLVM unit test failures. A few of them are unsafe ones so okay to be removed, but the remaining ones need updates in analysis or transformations to reenable them.
To minimize performance regression, what do you think about this workflow?
(1) This patch is landed to fix the miscompilation without significant regressions
(2) The clang noundef patch is enabled by default (I left a comment at its test-only patch)
(3) Flip `-instcombine-unsafe-select-transform` to false

Enabling the clang noundef flag reduces assembly diffs from LLVM test suite.
Without the noundef flag, the # of different assembly files is 737/5247 when `-instcombine-unsafe-select-transform` is flipped from true to false.
With the noundef flag, it becomes 632/5247.
It looks big, but about 80% of those are simply from reordering of basic blocks (I randomly picked 5 samples and 4 were such cases).
With this patch (D99674 <https://reviews.llvm.org/D99674>) only, the assembly diff # is already 439/5247, so 632 isn't very big.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99674