[PATCH] D84655: [InstSimplify] fold min/max intrinsics using icmp simplification

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D84655#2179046 <https://reviews.llvm.org/D84655#2179046>, @aqjune wrote:

> In D84655#2178544 <https://reviews.llvm.org/D84655#2178544>, @spatel wrote:
>
>> Yes, this reminds me of D77868 <https://reviews.llvm.org/D77868>, so I'm worried that we can't trust SimplifyICmpInst without a stronger undef check. Given that, I'd rather abandon this approach. I think we can put in a handful of direct min/max simplifications and be confident that we have good optimization and safety. Ie, back to the approach started with rG0481e1a <https://reviews.llvm.org/rG0481e1ae3c17a8faa6c8e319495a51a76b77b76b>.
>
> I think this case is a bit different from D77868 <https://reviews.llvm.org/D77868>: min/max can be expressed as a composition of icmp with select.
>
>   v = smax(a, b)
>   <=>
>   c = icmp sgt a, b
>   v = select c, a, b

The problem is that this does not hold. It's okay to go from icmp+select to max, but not the other way around. The difference is exactly that max requires a unique value for a and b, while the icmp+select sequence allows a and b to be chosen independently for the icmp and the select, if they are undef. This is also the reason why many of our existing min/max transforms are not actually legal when undef is taken into consideration.


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

https://reviews.llvm.org/D84655