Skip to content

[APFloat][NFC]extract fltSemantics::isRepresentableBy to header #122636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 13, 2025
+14 −11
Merged

[APFloat][NFC]extract fltSemantics::isRepresentableBy to header #122636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions llvm/include/llvm/ADT/APFloat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,11 @@ struct APFloatBase {
/// anything real.
static const fltSemantics &Bogus() LLVM_READNONE;

// Returns true if any number described by this semantics can be precisely
// represented by the specified semantics. Does not take into account
// the value of fltNonfiniteBehavior, hasZero, hasSignedRepr.
static bool isRepresentableBy(const fltSemantics &A, const fltSemantics &B);
Copy link
Member

@matthias-springer matthias-springer Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's your use case for this function? Given that this function does not take into account fltNonfiniteBehavior, hasZero, hasSignedRepr, I'm wondering how useful it is in practice. (I'm also wondering, why it does not take into account fltNonfiniteBehavior.)

Copy link
Contributor Author

@HerrCai0907 HerrCai0907 Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the usecase of this functions. I want to warn for implicit loss of accuracy when floating point conversation happened in compiler frontend type systems.
It needs API to detect floating point type compatibility.

(I'm also wondering, why it does not take into account fltNonfiniteBehavior.)

The original usage of this function is in APFloat::convertTo* series function to check compatibility.

At least for these cases, I think it only needs to check the floating point number compatibility in normal range instead to consider NaN and Inf.

Since this patch more like a refactor, I don't want to change the meaning of this API. Actually I try to add these requirements and test break.

Copy link
Contributor

@durga4github durga4github Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an NFC change, this looks good to me.

Copy link
Contributor

@durga4github durga4github Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are changing the semantics of this, can we do that as a separate PR?


/// @}

/// IEEE-754R 5.11: Floating Point Comparison Relations.
Expand Down
20 changes: 9 additions & 11 deletions llvm/lib/Support/APFloat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,6 @@ struct fltSemantics {

/* Whether this semantics can represent signed values */
bool hasSignedRepr = true;

// Returns true if any number described by this semantics can be precisely
// represented by the specified semantics. Does not take into account
// the value of fltNonfiniteBehavior.
bool isRepresentableBy(const fltSemantics &S) const {
return maxExponent <= S.maxExponent && minExponent >= S.minExponent &&
precision <= S.precision;
}
};

static constexpr fltSemantics semIEEEhalf = {15, -14, 11, 16};
Expand Down Expand Up @@ -290,6 +282,12 @@ const fltSemantics &APFloatBase::x87DoubleExtended() {
}
const fltSemantics &APFloatBase::Bogus() { return semBogus; }

bool APFloatBase::isRepresentableBy(const fltSemantics &A,
const fltSemantics &B) {
return A.maxExponent <= B.maxExponent && A.minExponent >= B.minExponent &&
A.precision <= B.precision;
Copy link
Member

@matthias-springer matthias-springer Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should take into account hasZero and hasSignedRepr. Or the documentation should make clear that it doesn't.

}

constexpr RoundingMode APFloatBase::rmNearestTiesToEven;
constexpr RoundingMode APFloatBase::rmTowardPositive;
constexpr RoundingMode APFloatBase::rmTowardNegative;
Expand Down Expand Up @@ -5527,7 +5525,7 @@ APFloat::opStatus APFloat::convertToInteger(APSInt &result,
double APFloat::convertToDouble() const {
if (&getSemantics() == (const llvm::fltSemantics *)&semIEEEdouble)
return getIEEE().convertToDouble();
assert(getSemantics().isRepresentableBy(semIEEEdouble) &&
assert(isRepresentableBy(getSemantics(), semIEEEdouble) &&
"Float semantics is not representable by IEEEdouble");
APFloat Temp = *this;
bool LosesInfo;
Expand All @@ -5541,7 +5539,7 @@ double APFloat::convertToDouble() const {
float128 APFloat::convertToQuad() const {
if (&getSemantics() == (const llvm::fltSemantics *)&semIEEEquad)
return getIEEE().convertToQuad();
assert(getSemantics().isRepresentableBy(semIEEEquad) &&
assert(isRepresentableBy(getSemantics(), semIEEEquad) &&
"Float semantics is not representable by IEEEquad");
APFloat Temp = *this;
bool LosesInfo;
Expand All @@ -5555,7 +5553,7 @@ float128 APFloat::convertToQuad() const {
float APFloat::convertToFloat() const {
if (&getSemantics() == (const llvm::fltSemantics *)&semIEEEsingle)
return getIEEE().convertToFloat();
assert(getSemantics().isRepresentableBy(semIEEEsingle) &&
assert(isRepresentableBy(getSemantics(), semIEEEsingle) &&
"Float semantics is not representable by IEEEsingle");
APFloat Temp = *this;
bool LosesInfo;
Expand Down
Loading