This is an archive of the discontinued LLVM Phabricator instance.

ValueTracking: Fix computeKnownFPClass handling for copysign
ClosedPublic

Authored by arsenm on Apr 25 2023, 1:14 PM.

Details

Summary

We need to expand the set of possible classes to the opposite
sign for the first operand if we don't know the sign of the second
operand.

Diff Detail

Event Timeline

arsenm created this revision.Apr 25 2023, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2023, 1:14 PM
arsenm requested review of this revision.Apr 25 2023, 1:14 PM
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
arsenm planned changes to this revision.Apr 25 2023, 2:52 PM

Something's broken with -0

arsenm updated this revision to Diff 516956.Apr 25 2023, 3:23 PM
arsenm updated this revision to Diff 516973.Apr 25 2023, 4:16 PM
foad added inline comments.Apr 26 2023, 1:23 AM
llvm/include/llvm/Analysis/ValueTracking.h
399–402

This doesn't work. Sign.isKnownNever(fcPositive) does not guarantee that the sign bit of Sign is 1, since it could be a nan with the sign bit clear.

I guess you meant Sign.isKnownNever(fcPositive | fcNan)? But this is another case where you could test Sign.SignBit && *Sign.SignBit as well or instead, since there is redundancy in the representation.

arsenm updated this revision to Diff 517128.Apr 26 2023, 4:53 AM
arsenm marked an inline comment as done.
foad added inline comments.Apr 26 2023, 5:11 AM
llvm/include/llvm/Analysis/ValueTracking.h
403

How do you know the result is not nan?

arsenm added inline comments.Apr 26 2023, 5:15 AM
llvm/include/llvm/Analysis/ValueTracking.h
403

You don’t. This is avoiding clearing the bits that say it could be nan

foad accepted this revision.Apr 26 2023, 5:22 AM
This revision is now accepted and ready to land.Apr 26 2023, 5:22 AM