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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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

> Hi,
> Is there a case where depending on SimplifyICmpInst may be problematic? I'm not very familiar with the history of miscompilation related with InstSimplify (except the distribution of undef), so it is possible that I'm missing something.
>
>   v = smax(x, y)
>   use(v)
>     => // isICmpTrue(sgt, x, y) is true
>   v = smax(x, y)
>   use(x) // Is this valid?
>
> For the D84250 <https://reviews.llvm.org/D84250>, the fix had to be conservative because distribution is incorrect under presence of undef. If InstSimplify can be arbitrarily smart so e.g. it iterates over the basic block and find the equivalent expression (as GVN does), this can cause miscompliation:
>
>   tmp = x + x
>   v = x * (1 + 1) // InstSimplify reaches to the conclusion that this is equivalent to x + x, finds tmp, so it uses tmp instead.
>   ->
>   tmp = x + x
>   v = tmp // This is wrong, because previously v is always an even, but not it can be anything if x is undef.

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>.


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

https://reviews.llvm.org/D84655