Page MenuHomePhabricator

[ValueTracking] ignore FP signed-zero when detecting a casted-to-integer fmin/fmax pattern
ClosedPublic

Authored by spatel on Dec 17 2017, 11:09 AM.

Details

Summary

This is a preliminary step for the patch discussed in D41136 (and denoted here with the FIXME comment).

When we match an FP min/max that is cast to integer, any intermediate difference between +0.0 or -0.0 should be muted in the result by the conversion (either fptosi or fptoui) of the result. Thus, we can enable 'nsz' for the purpose of matching fmin/fmax.

Diff Detail

Event Timeline

spatel created this revision.Dec 17 2017, 11:09 AM
fhahn accepted this revision.Dec 18 2017, 2:38 AM

LGTM thanks!

test/Transforms/InstCombine/minmax-fp.ll
163

Could you extend the test to also check the case when the select operands are flipped?

This revision is now accepted and ready to land.Dec 18 2017, 2:38 AM

LGTM as well.

Thinking about it, there are some other cases that will have a similar effect (e.g., adding/subtracting to a non-zero constant, squaring the result). Also, if the min/max is with a constant such that zero is not a possible answer. Maybe we could add a comment about that too?

LGTM as well.

Thinking about it, there are some other cases that will have a similar effect (e.g., adding/subtracting to a non-zero constant, squaring the result). Also, if the min/max is with a constant such that zero is not a possible answer. Maybe we could add a comment about that too?

Yes, there seems to be some more general opportunity to recognize FP min/max based on the surrounding uses. The more direct case with a non-zero constant is already handled via the bailout in the switch statement:

if (!FMF.noSignedZeros() && !isKnownNonZero(CmpLHS) &&
      !isKnownNonZero(CmpRHS))

Ie, when we have a non-zero constant, that check is false, so we keep going and look for the min/max pattern.

What seems odd in that check is that's a call that only checks for a non-zero constant value. That's not the more general call to the recursive:

static bool isKnownNonZero(const Value *V, unsigned Depth, const Query &Q);
spatel marked an inline comment as done.Dec 26 2017, 7:04 AM
This revision was automatically updated to reflect the committed changes.