This is an archive of the discontinued LLVM Phabricator instance.

ValueTracking: fdiv sign handling in computeKnownFPClass
ClosedPublic

Authored by arsenm on Apr 18 2023, 5:52 AM.

Details

Summary

Copy what cannotBeOrderedLessThanZeroImpl checks for fdiv.

Diff Detail

Event Timeline

arsenm created this revision.Apr 18 2023, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2023, 5:52 AM
arsenm requested review of this revision.Apr 18 2023, 5:52 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: wdng. · View Herald Transcript
kpn accepted this revision.Apr 18 2023, 11:09 AM

Seems reasonable. LGTM.

This revision is now accepted and ready to land.Apr 18 2023, 11:09 AM
foad added inline comments.Apr 19 2023, 1:23 AM
llvm/include/llvm/Analysis/ValueTracking.h
367–369
llvm/lib/Analysis/ValueTracking.cpp
4660

WantInfNan || WantNegative is true, else we would have bailed out

4676–4678

This may be true but it seems weirdly specific, and the comment doesn't really say what it does.

I think it generalises to:

if (KnownLHS.isKnownNot(fcNegative) && KnownRHS.isKnownNot(fcNegative))
  Known.knownNot(fcNegative);

... plus three other cases for the other combinations of LHS/RHS known not positive/negative.

This is basically saying that ignoring nans, the sign of the result is the xor of the signs of the arguments, which is also true for FMul.

arsenm updated this revision to Diff 514932.Apr 19 2023, 6:09 AM
arsenm marked 3 inline comments as done.
foad added inline comments.Apr 20 2023, 3:04 AM
llvm/lib/Analysis/ValueTracking.cpp
4647
4654
4658–4659

neverInfinity and neverZero are not useful on their own, only if you already know neverNan.

4663
arsenm requested review of this revision.Apr 24 2023, 1:49 PM
arsenm updated this revision to Diff 516525.
arsenm marked an inline comment as done.
foad accepted this revision.Apr 25 2023, 2:41 AM

LGTM with comment nit.

llvm/lib/Analysis/ValueTracking.cpp
4676

Comment should say +ve / +ve is +ve.

This revision is now accepted and ready to land.Apr 25 2023, 2:41 AM
arsenm added inline comments.Apr 25 2023, 4:27 AM
llvm/lib/Analysis/ValueTracking.cpp
4676

ve?

foad added inline comments.Apr 25 2023, 4:48 AM
llvm/lib/Analysis/ValueTracking.cpp
4676

"+ve" is short for "positive". I just mean the comment should describe what the code is doing, instead of just mentioning a true but largely irrelevant fact.