[PATCH] D124526: [SimpleLoopUnswitch] Collect either logical ANDs/ORs but not both.

Sun May 1 00:52:16 PDT 2022

aqjune added a comment.

In D124526#3483907 <https://reviews.llvm.org/D124526#3483907>, @fhahn wrote:

> In D124526#3483889 <https://reviews.llvm.org/D124526#3483889>, @aqjune wrote:
>
>> There was a similar miscompilation report (which was introduced by my patch) and a bug fix due to the existence of `select cond, true, false` in the past: https://reviews.llvm.org/rG5bb38e84d3d0#986154 and https://reviews.llvm.org/rG6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0
>> Also a follow-up patch to enhance an assertion condition: https://reviews.llvm.org/rG431a40e1e28f181e87dd247b91a5e6872dd64ab4
>>
>>> This only fixes trivial unswitching for now, but a similar problem
>>> likely exists with non-trivial unswitching.
>>
>> If the `select cond, true, false` is a concern, what do you think about simplifying it to `cond` (https://alive2.llvm.org/ce/z/p5Ahrp) in advance before loop unswitch?
>
> Is it possible you did not check the latest version of the patch? It uses helpers to skip `select cons, true, false` when trying to match logical and/or.
>
> I don't think we really should simplify/modify the IR if we are not unswitching, which is what 6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0 <https://reviews.llvm.org/rG6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0> did, but SimpleLoopUnswitch claims no changes have been made.

Oh, I totally forgot the conclusion that were discussed in the threads; I recalled that the `select` form was problematic in the past, but not discussions afterwards.
Even the branch condition-changing code (line 2754~) was still there. Thank you for cleaning up the code bit.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124526