Page MenuHomePhabricator

Combine fcmp + select to fminnum / fmaxnum if no nans and legal
ClosedPublic

Authored by arsenm on Dec 19 2014, 3:29 PM.

Details

Reviewers
ab

Diff Detail

Event Timeline

arsenm updated this revision to Diff 17525.Dec 19 2014, 3:29 PM
arsenm retitled this revision from to Combine fcmp + select to fminnum / fmaxnum if no nans and legal .
arsenm updated this object.
arsenm edited the test plan for this revision. (Show Details)
arsenm set the repository for this revision to rL LLVM.
arsenm added a subscriber: Unknown Object (MLST).
arsenm added a reviewer: ab.Jan 12 2015, 1:30 PM
ab edited edge metadata.Jan 12 2015, 3:31 PM

The NaN part LGTM, but what happens on 0?

Say:

select +0.0, -0.0, (fcmp lt +0.0, -0.0)

turns into:

fminnum +0.0, -0.0

My understanding is, the first returns -0.0 (because they compare equal, and not "lt").
But it's unspecified which the second returns, so if the implementation returns the first operand when both are zero, it would return a "different" result here, +0.0.
Does that make sense?

-Ahmed

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4629

Nit: clang-format? (or at least the same formatting as the call, with e.g. LHS/RHS on the same line.)

4742

Optionally, how about checking isKnownNeverNaN for both operands?
Also, the non-U/-O CondCodes don't care about NaNs, so it would be fine to bypass NoNaNsFPMath in that case as well, no? Though I'm not sure that combination ever happens anyway.

4749

Nit: same, format?

In D6744#107740, @ab wrote:

The NaN part LGTM, but what happens on 0?

Say:

select +0.0, -0.0, (fcmp lt +0.0, -0.0)

turns into:

fminnum +0.0, -0.0

My understanding is, the first returns -0.0 (because they compare equal, and not "lt").
But it's unspecified which the second returns, so if the implementation returns the first operand when both are zero, it would return a "different" result here, +0.0.
Does that make sense?

Yes, however I am unclear on what guarantees there are for signed zeros. The DAG TargetOptions are also missing the equivalent of No Signed Zeros. I can weaken this to unsafe FP Math as well.

-Ahmed

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4742

I didn't know about isKnownNeverNaN, I'll switch to using it.

They do care about NaN. The ordered compares fail if either operand is a NaN, and the unordered succeed if either is.

ab added a subscriber: resistor.Jan 12 2015, 3:58 PM

+Owen

In D6744#107740, @ab wrote:

The NaN part LGTM, but what happens on 0?

Say:

select +0.0, -0.0, (fcmp lt +0.0, -0.0)

turns into:

fminnum +0.0, -0.0

My understanding is, the first returns -0.0 (because they compare equal, and not "lt").
But it's unspecified which the second returns, so if the implementation returns the first operand when both are zero, it would return a "different" result here, +0.0.
Does that make sense?

Yes, however I am unclear on what guarantees there are for signed zeros. The DAG TargetOptions are also missing the equivalent of No Signed Zeros. I can weaken this to unsafe FP Math as well.

I believe the X86 equivalent of this combine swaps operands to make sure it gets the same result, but that's only possible because the operation is specified there.

Other knowledgeable FP people, an opinion?

-Ahmed

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4742

I meant the non-orderered, and non-unordered, compares, as e.g., ISD::SETLT, ISD::SETGT. Those are undefined on NaNs, no?

arsenm updated this revision to Diff 18059.Jan 12 2015, 4:02 PM
arsenm edited edge metadata.
arsenm removed rL LLVM as the repository for this revision.

Use isKnownNeverNaN, check UnsafeFPMath due to signed zeros

ab accepted this revision.Jan 12 2015, 4:21 PM
ab edited edge metadata.

LGTM

-Ahmed

This revision is now accepted and ready to land.Jan 12 2015, 4:21 PM
arsenm closed this revision.Jan 12 2015, 4:44 PM

r225744

mehdi_amini added inline comments.Jan 12 2015, 5:40 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4630

What about renaming True TrueValue and False FalseValue or something like that. I was confused with c++ true/false keyword when reading this in the first place.

4659

Can't you move the code out of the switch and having the switch only initializing the OpCode?

4742

Isn't the transformation valid for SETGT, SETGE, SETLT, SETLE even without Options.NoNaNsFPMath?

4742

Why is the limit N0.hasOneUse()?

4749

It is the correct way of indenting the arg list here?

arsenm added inline comments.Jan 12 2015, 5:47 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4659

Yes, but I don't like assigning variables like that and having to track them

4742

Oh, those. Yes, i it would be OK for those. However, I don't think I've ever actually seen one of those produced for FP compares before.

4749

This is what clang-format produced