[PATCH] D107148: [InstCombine] Fold two-value clamp patterns

Fri Jul 30 06:43:12 PDT 2021

dmgreen added a comment.

In D107148#2916113 <https://reviews.llvm.org/D107148#2916113>, @lebedev.ri wrote:

> In D107148#2916097 <https://reviews.llvm.org/D107148#2916097>, @mkazantsev wrote:
>
>> The initial code
>>
>>   // s1 = (n < C1) ? n : C1
>>   // s2 = (s1 > C1 - 1) ? s1 : C1 - 1
>>
>> had a clear semantics of
>>
>>   s1 = min(n, C1)
>>   s2 = max(s1, C1 - 1)
>>
>> These pattern could be analyzed by other passes that understand semantics of min and max, such as SCEV using passes. If it participates in other min or max expression, there is a lot of ways how we can simplify it.
>>
>> This new form, despite it has fewer instructions, is completely arcane. Just looking at it, it's not so easy to understand what it actually means. And no pass will.
>>
>> I'd rather suggest to do changes like this as close to codegen as possible, for example in CodeGenPrepare.
>
> Sounds like SCEV should be aware of this way to parse such a select-of-constants as min-max.

I know it's a different case, but I would consider the saturation case to be canonical as:

  %m = call i32 @llvm.smin.i32(i32 %num, i32 127)
  %r = call i32 @llvm.smax.i32(i32 %m, i32 -128)

And I believe we have code that relies on that.

But if this does indeed simplify to

  %1 = icmp sgt i32 %0, 126
  %r = select i1 %1, i32 127, i32 126

that doesn't sound worse than the min and max in this case.

The code might be better if it made use of matchSelectPattern. There are "SPF" matchers for matching min/max patterns, like in factorizeMinMaxTree and moveNotAfterMinMax.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107148