What does this comment in InstCombineSelect refer to?

/// TODO: Also support a - UMIN(a,b) patterns.

This is some old version of already commited patch, I think we need to close this.

In D25987#1005390, @spatel wrote:

Did D41480 affect anything with these tests? We still need to handle 'umin' patterns in IR and update the tests here with canonical IR forms to show psubus codegen?

Looks like all possible umin patterns are already handled in existing patterns we added for max. The last tests in psubus.ll is also cannonical cmp-select-sub sequence. What should I add which is missing?

Fixed last comments. Thank you for your help, could you please commit it for me?

Thank you for your comments, fixed them.

Fixed the comments

Fixed comments,

In D25987#908551, @RKSimon wrote:

@yulia_koval What is the state of this after D37510 and D37534?

Thanks, fixed your comment in additional patch below.

Thanks! Could you please commit it for me?

Fixed problem on min case.

In D37510#882071, @zvi wrote:

LGTM

Added 512bit support and fixed comments

Added 512bit support.

In D37510#863372, @spatel wrote:

Is this patch intended to just add the baseline tests, or is there an associated code change that didn't get uploaded?

Moved second patch(backend part) to extra revision for convenience https://reviews.llvm.org/D37534.

tests, rebased

I can't commit, could you commit this for me please?

Tried this, it caused other changes, is it ok?

In D37510#862264, @zvi wrote:

Please update the Summary with more details about the combine that was added.

This is the second patch, containing backend-code for new pattern.

In D32643#756257, @spatel wrote:

Do you need someone to commit this for you? I just hit a case where another transform would affect codegen in this file, so I'd like to make sure these tests are minimal.

In D25987#743482, @spatel wrote:

But do you agree that it is reasonable to write this function in C as:

void goo(unsigned short *p, int max, int n) {
  int i;
  unsigned m;
  for (i = 0; i < n; i++) {
    m = *--p;
    unsigned umax = m > max ? m : max;
    *p = (unsigned short)(umax - max);
  }
}

In D25987#742378, @spatel wrote:

I think we need to take a step back here. The tests are assuming a specific form of IR, but I'm not sure it's canonical. If we change this upstream (in InstCombine), the transform may fail to activate when you hoped it would. It's also possible that generic DAG combines could interfere with the pattern you're expecting. See rL301781 as an example. In either case, this seems like a fragile solution because the pattern can change above this.

http://rise4fun.com/Alive/5e4
Unless I made a mistake in transcribing that, the 1st form is what you're expecting in these tests. The 2nd form might be considered better since it includes a canonical 'max'.

Can you test this patch with more examples like this to see if it behaves as expected:

define i16 @scalar(i16 %x, i32 %y) nounwind {
  %lhs = zext i16 %x to i32
  %cond = icmp ugt i32 %lhs, %y
  %sub = sub i32 %lhs, %y
  %truncsub = trunc i32 %sub to i16
  %res = select i1 %cond, i16 %truncsub, i16 zeroinitializer
  ret i16 %res
}

define i16 @scalar_max(i16 %x, i32 %y) nounwind {
  %lhs = zext i16 %x to i32
  %cond = icmp ugt i32 %lhs, %y
  %umax = select i1 %cond, i32 %lhs, i32 %y
  %sub = sub i32 %umax, %y
  %truncsub = trunc i32 %sub to i16
  ret i16 %truncsub
}

define <8 x i16> @test15(<8 x i16> %x, <8 x i32> %y) nounwind {
  %lhs = zext <8 x i16> %x to <8 x i32>
  %cond = icmp ugt <8 x i32> %lhs, %y
  %sub = sub <8 x i32> %lhs, %y
  %truncsub = trunc <8 x i32> %sub to <8 x i16>
  %res = select <8 x i1> %cond, <8 x i16> %truncsub, <8 x i16> zeroinitializer
  ret <8 x i16> %res
}

define <8 x i16> @test15_max(<8 x i16> %x, <8 x i32> %y) nounwind {
  %lhs = zext <8 x i16> %x to <8 x i32>
  %cond = icmp ugt <8 x i32> %lhs, %y
  %umax = select <8 x i1> %cond, <8 x i32> %lhs, <8 x i32> %y
  %sub = sub <8 x i32> %umax, %y
  %truncsub = trunc <8 x i32> %sub to <8 x i16>
  ret <8 x i16> %truncsub
}

define <8 x i8> @test15_narrow(<8 x i8> %x, <8 x i16> %y) nounwind {
  %lhs = zext <8 x i8> %x to <8 x i16>
  %cond = icmp ugt <8 x i16> %lhs, %y
  %sub = sub <8 x i16> %lhs, %y
  %truncsub = trunc <8 x i16> %sub to <8 x i8>
  %res = select <8 x i1> %cond, <8 x i8> %truncsub, <8 x i8> zeroinitializer
  ret <8 x i8> %res
}

define <8 x i8> @test15_narrow_max(<8 x i8> %x, <8 x i16> %y) nounwind {
  %lhs = zext <8 x i8> %x to <8 x i16>
  %cond = icmp ugt <8 x i16> %lhs, %y
  %umax = select <8 x i1> %cond, <8 x i16> %lhs, <8 x i16> %y
  %sub = sub <8 x i16> %umax, %y
  %truncsub = trunc <8 x i16> %sub to <8 x i8>
  ret <8 x i8> %truncsub
}

No, I can actually update all the tests like this.

In D25987#673846, @spatel wrote:

It's difficult for me to see what's going on in these tests because there are a bunch of irrelevant load/store/gep/bitcast instructions. Can you remove those?

If you do that, the root of the problem is very similar to:
https://reviews.llvm.org/rL286776

Ie, if we recognized the umin/umax patterns in IR as ISD::UMIN/ISD::UMAX when we created the DAG, the subsequent matching to the x86-specific PSUBUS might already happen with the existing lowering?

Gentle ping

Sorry, fixed clang-format.

I updated the patch to trunk, because test output changed due to rL292479 optimization. I failed to find a better check, based on TLI.isLegal.., because it doesn't requires exact type. For example, in case of SSE4.1, isOperationLegalOrCustom(MIN, v8i32) returns false and disables the optimization. However, the transformation works fine as the illegal operation is split into two v4i32 operations. The hasSSE41 check is sufficient, because it includes both v8i16 and v4i32 for each integer type. i8(SSE hass i8 based umin) can't be used in this optimisation, because the ExtType is always larger then psubus type and can be minimum i16.

This optimization requires the umin instruction. It didn't exist before SSE4.1, so I added requirement for SSE4 for it and added new target to test. I haven't found profitable equivalent for umin in sse\sse2 for this situation.

1. Changed regex-based test to generated from update_llc_test_checks.py script
2. Replaced getScalarType().getSizeInBits() with VT.getScalarSizeInBits()

Gentle ping