[PATCH] D87571: [DAGCombiner] Fold fmin/fmax with INF

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D87571#2270204 <https://reviews.llvm.org/D87571#2270204>, @spatel wrote:

> LGTM. Should we common/integrate the caller functions/switch cases? Ie, we're switching on min/max opcode, but then translating that knowledge into the bool flags and then predicating on those flags instead of the opcodes.

I've kept the flags but moved their definition into visitFMinMax, based on the opcode. Does that look reasonable? I think unpacking into the two flags reads a bit nicer than checking opcodes everywhere.

> Comparing this to InstSimplify...I think we have 2 problems there:
>
> 1. D52766 <https://reviews.llvm.org/D52766> introduced a miscompile for maximum/minimum (need more tests)
> 2. Optimizations are missing (noted with TODO comments)
>
> There's also another set of potential optimization to include here (based on ninf):
>
>   // TODO: minnum(nnan ninf x, flt_max) -> x
>   // TODO: maxnum(nnan ninf x, -flt_max) -> x
>
> Let me know if you plan to work on those. If not, I can do it.

For the sake of completeness, I've extended this patch to handle the +/-flt_max case as well. I'd be happy to leave the InstSimplify part of this to you though :)


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

https://reviews.llvm.org/D87571