Page MenuHomePhabricator

[DAGCombiner] Fold fmin/fmax with INF
ClosedPublic

Authored by nikic on Sep 12 2020, 3:27 PM.

Details

Summary

Similar to D87415, this folds the various float min/max opcodes with a constant INF or -INF operand, or FLT_MAX / -FLT_MAX operand if the ninf flag is set. Some of the folds are only possible under nnan.

The fminnum(X, INF) with nnan and fmaxnum(X, -INF) with nnan cases are needed to improve the VECREDUCE_FMIN/FMAX lowerings on X86, the rest is here for the sake of completeness.

Diff Detail

Event Timeline

nikic created this revision.Sep 12 2020, 3:27 PM
nikic requested review of this revision.Sep 12 2020, 3:27 PM
spatel accepted this revision.Sep 13 2020, 6:42 AM

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.

Comparing this to InstSimplify...I think we have 2 problems there:

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

This revision is now accepted and ready to land.Sep 13 2020, 6:42 AM
nikic updated this revision to Diff 291458.Sep 13 2020, 8:10 AM
nikic edited the summary of this revision. (Show Details)

Also fold FLT_MAX/-FLT_MAX if ninf flag is set.

nikic added a comment.Sep 13 2020, 8:13 AM

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 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 :)

nikic updated this revision to Diff 291460.Sep 13 2020, 8:26 AM

Fix confusion between isSmallest and isLargest+isNegative.

spatel added inline comments.Sep 13 2020, 12:52 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14058–14060

There's a bug here that should be addressed first (probably lacking test coverage) - we dropped the FMF when creating the new node.
It would be more efficient to std::swap(N0, N1) and just use those local names in all of the later code?

14077–14078

If we're using the same order of functions as in the code comment above here, the max/min names are inverted.

14085

"+inf" should be "-inf" on this line?

nikic updated this revision to Diff 291472.Sep 13 2020, 1:36 PM

Preserve flags when commuting operands, fix typos in comments.

nikic marked 3 inline comments as done.Sep 13 2020, 1:44 PM
nikic added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14058–14060

I added some tests with nnan and commuted operands. Turns out that for the scalar case this canonicalization happens somewhere else already (maybe node building already handles it?) and this code only affects the vector constant case.

I've kept the code (with fmf propagation) as the same canonicalization seems to also be done for other commutative nodes.

spatel accepted this revision.Sep 14 2020, 5:40 AM

Expanded patch still LGTM.
Side note: change "isLargest" name to "isLargestMagnitude" to make that API less confusing?

This revision was automatically updated to reflect the committed changes.
nikic marked an inline comment as done.