[PATCH] D74484: [AggressiveInstCombine] Add support for ICmp instr that feeds a select intsr's condition operand.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Sorry, I did not explain my previous comment as well as possible. Let's take this minimal example:

  define i1 @f(i8 %a) {
    %conv = sext i8 %a to i32
    %cmp = icmp slt i32 %conv, 109
    ret i1 %cmp
  }

With instcombine today, we can reduce this as:

  define i1 @f(i8 %a) {
    %cmp = icmp slt i8 %a, 109
    ret i1 %cmp
  }

We can also see that with instcombine today, we can reduce this example even if the cast (sext in this example) has other uses:

  declare void @use(i32)
  define i1 @f(i8 %a) {
    %conv = sext i8 %a to i32
    call void @use(i32 %conv)
    %cmp = icmp slt i32 %conv, 109
    ret i1 %cmp
  }

Narrow 'cmp' here even though the sext is not eliminated:

  define i1 @f(i8 %a) {
    %conv = sext i8 %a to i32
    call void @use(i32 %conv)
    %cmp = icmp slt i8 %a, 109
    ret i1 %cmp
  }

So that's what I was trying to suggest - the limitation is not entirely about the multiple uses in these small examples. The 1st regression test example would have been altered by rGf2fbdf76d8d0 <https://reviews.llvm.org/rGf2fbdf76d8d07f6a0fbd97825cbc533660d64a37> to become:

  define i16 @cmp_select_sext_const(i8 %a) {
    %c = icmp sgt i8 %a, 109
    %narrow = select i1 %c, i8 %a, i8 109
    %conv4 = zext i8 %narrow to i16
    ret i16 %conv4
  }

And even with an extra use of the sext, we would get the narrow icmp/select:

  define i16 @cmp_select_sext_const(i8 %a) {
    %conv = sext i8 %a to i32
    call void @use(i32 %conv)
    %1 = icmp sgt i8 %a, 109
    %narrow = select i1 %1, i8 %a, i8 109
    %conv4 = zext i8 %narrow to i16
    ret i16 %conv4
  }

In fact, even when the select also has an extra use (you can reproduce this on current LLVM: https://godbolt.org/z/P6Kws_ ):

  define i16 @cmp_select_sext_const(i8 %a) {
    %conv = sext i8 %a to i32
    call void @use(i32 %conv)
    %cmp = icmp slt i8 %a, 109
    %cond = select i1 %cmp, i32 109, i32 %conv
    call void @use(i32 %cond)
    %conv4 = trunc i32 %cond to i16
    ret i16 %conv4
  }

Instcombine will produce this (which seems unintended because we increased the instruction count):

  define i16 @cmp_select_sext_const(i8 %a) {
    %conv = sext i8 %a to i32
    call void @use(i32 %conv)
    %1 = icmp sgt i8 %a, 109
    %narrow = select i1 %1, i8 %a, i8 109
    %cond = zext i8 %narrow to i32
    call void @use(i32 %cond)
    %conv4 = zext i8 %narrow to i16
    ret i16 %conv4
  }

So I'm suggesting that we cover that last case in addition to the simpler test, so we will know what happens with multiple extra uses.

Also note that we have floated the idea again of adding min/max intrinsics to IR in D68408 <https://reviews.llvm.org/D68408>. Obviously, we need to post that as a proposal on llvm-dev for wider review...but if we had those intrinsics in IR, it probably affects at least some of the motivation for this patch, so I'd be interested in your thoughts on that.


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

https://reviews.llvm.org/D74484