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

Sat Apr 3 12:15:35 PDT 2021

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

In D99674#2664219 <https://reviews.llvm.org/D99674#2664219>, @aqjune wrote:

> 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.

I'm OK with landing this first to fix an active miscompile, but I don't think we should block flipping the switch on noundef work. I think your numbers support that we shouldn't wait on that, because the noundef impact it small relative to the whole change (it only makes a 15% difference in affected files).

================
Comment at: llvm/test/Transforms/PhaseOrdering/unsigned-multiply-overflow-check.ll:116
+; INSTCOMBINESIMPLIFYCFGINSTCOMBINE-NEXT:    [[PHI_BO:%.*]] = xor i1 [[UMUL_OV]], true
+; INSTCOMBINESIMPLIFYCFGINSTCOMBINE-NEXT:    ret i1 [[PHI_BO]]
 ;
----------------
This test update looks unnecessary? Only changes names.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99674