Page MenuHomePhabricator

[InstCombine] canonicalize fcmp+select to minnum/maxnum intrinsics
ClosedPublic

Authored by spatel on May 24 2019, 11:34 AM.

Details

Summary

This is the opposite direction of D62158 (we have to choose 1 form or the other). Now that we have FMF on the select, this becomes more palatable. And the benefits of having a single IR instruction for this operation (less chances of missing folds based on extra uses, etc) overcome my previous comments about the potential advantage of larger pattern matching/analysis. I'll abandon the other patch if there's general agreement.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.May 24 2019, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 11:34 AM
llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

Do we want NSZ here? I see that this was an artifact of the old fcmp+select lowering, but I'm not sure if it is correct. Definitely not IEEE-754 compliant...

llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

Now that I think about it, maybe it would be better to skip the fcmp+sel step and lower the libm call directly into the intrinsic?

spatel marked an inline comment as done.Jun 11 2019, 8:15 AM
spatel added inline comments.
llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

Can you explain/show example for the compliancy problem?

Reference for our current docs:
http://llvm.org/docs/LangRef.html#llvm-minnum-intrinsic

llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

Ah, right. This is ok in IEEE-754 2008. My copy of IEEE-754 2018 (a draft):

minimumNumber(x, y) is x if x < y, y if y < x, and the number if one operand is a number and the 25 other is a NaN. !For this operation, −0 compares less than +0.! If x = y and signs are the same it is either x or y. If both operands are NaNs, a quiet NaN is returned, according to 6.2. If either operand is a signaling NaN, an invalid operation exception is signaled, but unless both operands are NaNs, the signaling NaN is otherwise ignored and not converted to a quiet NaN as stated in 6.2 for other operations.

So adding NSZ there wouldn't be correct for the min(-0,+0) and friends cases.

spatel marked an inline comment as done.Jun 11 2019, 8:53 AM
spatel added inline comments.
llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

I might still be missing some subtlety.

My interpretation of the LangRef text (and so also IEEE-754 2008) is that 'nsz' ("Allow optimizations to treat the sign of a zero argument or result as insignificant") is implicit in the definition of minnum/maxnum.

By explicitly propagating the 'nsz' in this case, we are future-proofing the behavior even for the new standard. The original fcmp+select said that sign of zero is a 'don't care', and we want the intrinsic call to have that same freedom.

cameron.mcinally accepted this revision.Jun 11 2019, 9:17 AM
cameron.mcinally marked an inline comment as done.

LGTM

llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

Yeah, I agree your new code is fine.

I was questioning whether we should be adding NSZ during the @fmin(...) lowering, since the NSZ isn't on the original fmin(...) call. But now I don't see anything that requires @fmin*(...) to honor the sign of a zero in the Standards or libm. So this is fine too.

Sorry for the noise...

This revision is now accepted and ready to land.Jun 11 2019, 9:17 AM
spatel marked an inline comment as done.Jun 11 2019, 9:23 AM
spatel added inline comments.
llvm/test/Transforms/InstCombine/fast-math.ll
884 ↗(On Diff #201295)

Ah, I see now. Not noise at all - these are excellent and valid points.

It goes back to your other comment - we could be transforming the fmin call directly to the intrinsic in these examples. So that confusion was just caused by me being lazy and not creating minimal tests for this patch. Let me do that, then I'll try to fix up that existing transform.

This revision was automatically updated to reflect the committed changes.