Page MenuHomePhabricator

[Constants][PowerPC] Check exactlyValue for ppc_fp128 in isNullValue
ClosedPublic

Authored by jsji on Jun 3 2021, 10:45 AM.

Details

Summary

PPC_FP128 determines isZero/isNan/isInf using high-order double value
only. Checking isZero/isNegative might return the isNullValue unexpectedly.
eg:

0xM0000000000000000FFFFFFFFFFFFFFFFF

isZero, but it is not NullValue.

Diff Detail

Event Timeline

jsji created this revision.Jun 3 2021, 10:45 AM
jsji requested review of this revision.Jun 3 2021, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2021, 10:45 AM
jsji edited the summary of this revision. (Show Details)Jun 3 2021, 10:45 AM
jsji added a reviewer: timshen.
jsji retitled this revision from [BitcodeWriter][PowerPC] Avoid clearing lower bits for NullValues to [BitcodeWriter][PowerPC] Avoid clearing lower bits for ppc_fp128 NullValues.Jun 3 2021, 10:47 AM

If Constant::isNullValue() isn't returning the right result, please fix it directly. Getting it wrong could have other unexpected consequences.

jsji added a comment.Jun 3 2021, 11:17 AM

If Constant::isNullValue() isn't returning the right result, please fix it directly. Getting it wrong could have other unexpected consequences.

Thanks @efriedma for the comments.

In this case Constant::isNullValue() is correct, as the value is indeed a NullValue for ppc_fp128.
per definition of a long double in 3.1.5. Extended Precision https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html

Yes, we will need to deal with this special cases carefully in other optimizations as well to avoid unexpected consequences.

The fix is for BitcodeWriters, so that we won't end up clear bits unexpected, which causes unexpected different behaviors using bitcode vs. IR.

Constant::isNullValue() is supposed to mean "is this value all zero bits"; various parts of the code use it that way. It shouldn't care whether the value is semantically equal to zero. Anything doing floating-point arithmetic should be using the appropriate ConstantFP/APFloat methods.

jsji added a comment.Jun 3 2021, 11:33 AM

Constant::isNullValue() is supposed to mean "is this value all zero bits"; various parts of the code use it that way. It shouldn't care whether the value is semantically equal to zero. Anything doing floating-point arithmetic should be using the appropriate ConstantFP/APFloat methods.

Oh... Thanks for the info, I will check and update.

jsji updated this revision to Diff 349648.Jun 3 2021, 12:35 PM

Fix isNullValue for ppc_fp128.

jsji retitled this revision from [BitcodeWriter][PowerPC] Avoid clearing lower bits for ppc_fp128 NullValues to [Constants][PowerPC] Check exactlyValue for ppc_fp128 in isNullValue.Jun 3 2021, 12:38 PM
jsji edited the summary of this revision. (Show Details)
jsji added a reviewer: efriedma.
efriedma accepted this revision.Jun 3 2021, 12:52 PM

LGTM with one minor comment.

llvm/lib/IR/Constants.cpp
96

We can use isExactlyValue() for all floating-point types; no reason to have two separate codepaths.

This revision is now accepted and ready to land.Jun 3 2021, 12:52 PM
jsji updated this revision to Diff 349664.Jun 3 2021, 1:03 PM

Use isExactlyValue() for all floating-point types.

jsji marked an inline comment as done.Jun 3 2021, 1:04 PM
This revision was landed with ongoing or failed builds.Jun 3 2021, 1:31 PM
This revision was automatically updated to reflect the committed changes.