[PATCH] D111804: [InstCombine] Don't combine CmpInst that used for verify the legality of the lshr

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D111804#3065924 <https://reviews.llvm.org/D111804#3065924>, @TiehuZhang wrote:

> In D111804#3065906 <https://reviews.llvm.org/D111804#3065906>, @craig.topper wrote:
>
>> If an input to an `or` instruction is poison, the output of the `or` is poison regardless of whether the other operand is true when the poison result occurs.
>>
>> If an incorrect optimization is occurring it must be where the `or` is created.
>
> Thanks for your review! @craig.topper. Do you mean that we should prevent this pattern from being generated, rather than remedying it after the pattern is generated?  Actually, the issue I met started from D95959 <https://reviews.llvm.org/D95959>. That optimization does not seem to apply to all scenarios, such as those where the shift operand is negative. In the case I present, this optimization results in a miscalculation of the KnownBit, which in turn results in a false transformation.

The transform is correct based on the semantics of llvm's `or` instruction. The `or` instruction in llvm does not have short circuit evaluation. It can't block the spread of poison. The IR that has the semantics you want is

  define i32 @cmp_lshr_cmp(i32 %0) {
  entry:
    %cmp = icmp slt i32 %0, 0
    %shr = lshr i32 7, %0
    %cmp1 = icmp sgt i32 %0, %shr
    %or.cond = select i1 %cmp, i1 true, i1 %cmp1
    br i1 %or.cond, label %cond.end, label %cond.false
  
  cond.false:                                       ; preds = %entry
    ret i32 1
  
  cond.end:                                         ; preds = %entry, %cond.false
    ret i32 0
  }

Using a select instruction instead of an `or` prevents the %cmp1 value from being used if %cmp is true. There was previously an incorrect transform that would turn such a select into an `or`, but it was disabled in https://reviews.llvm.org/D101191


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111804