Update the EBO static_assert to dheck against APFloat::Storage instead of IEEEFloat.

[DanielCChen]

The addition of the the following code in commit 6004f55#diff-3ec11da67f69353fd755e59cc71f551b5e93773fd4e6f6327c7f1c7074a82709R1474

static_assert(sizeof(APFloat) == sizeof(detail::IEEEFloat),
              "Empty base class optimization is not performed.");

broke our downstream code as not all the members of APFloat is IEEE.

This PR supersedes PR #111780.

We made some changed to our downstream code to accommodate the upstream change.
This PR is to modify the EBO static_assert to check against the sizeof(APFloat::Storage) instead of sizeof(detail::IEEEFloat), which seems satisfy both upstream and downstream code.
We also moved the assert to APFloat.cpp from the header file as per discussion in PR #111780

@Ariel-Burton

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Daniel Chen (DanielCChen)

Changes

The addition of the the following code in commit 6004f55#diff-3ec11da67f69353fd755e59cc71f551b5e93773fd4e6f6327c7f1c7074a82709R1474

static_assert(sizeof(APFloat) == sizeof(detail::IEEEFloat),
              "Empty base class optimization is not performed.");

broke our downstream code as not all the members of APFloat is IEEE.

This PR supersedes PR #111780.

We made some changed to our downstream code to accommodate the upstream change.
This PR is to modify the EBO static_assert to check against the sizeof(APFloat::Storage) instead of sizeof(detail::IEEEFloat), which seems satisfy both upstream and downstream code.
We also moved the assert to APFloat.cpp from the header file as per discussion in PR #111780

@Ariel-Burton

---

Full diff: https://github.com/llvm/llvm-project/pull/112589.diff

2 Files Affected:

• (modified) llvm/include/llvm/ADT/APFloat.h (+1-3)
• (modified) llvm/lib/Support/APFloat.cpp (+5)

diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index 97547fb577e0ec..85e17efd1fba8e 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -1474,11 +1474,9 @@ class APFloat : public APFloatBase {
   friend APFloat frexp(const APFloat &X, int &Exp, roundingMode RM);
   friend IEEEFloat;
   friend DoubleAPFloat;
+  friend class APFloatEBOChecker;
 };
 
-static_assert(sizeof(APFloat) == sizeof(detail::IEEEFloat),
-              "Empty base class optimization is not performed.");
-
 /// See friend declarations above.
 ///
 /// These additional declarations are required in order to compile LLVM with IBM
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index a33b6c4a6ddc63..f48c82b6afdd89 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -99,6 +99,11 @@ enum class fltNanEncoding {
   NegativeZero,
 };
 
+class APFloatEBOChecker {
+  static_assert(sizeof(APFloat) == sizeof(APFloat::U),
+                "Empty base class optimization is not performed.");
+};
+
 /* Represents floating point arithmetic semantics.  */
 struct fltSemantics {
   /* The largest E such that 2^E is representable; this matches the

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Daniel Chen (DanielCChen)

Changes

The addition of the the following code in commit 6004f55#diff-3ec11da67f69353fd755e59cc71f551b5e93773fd4e6f6327c7f1c7074a82709R1474

static_assert(sizeof(APFloat) == sizeof(detail::IEEEFloat),
              "Empty base class optimization is not performed.");

broke our downstream code as not all the members of APFloat is IEEE.

This PR supersedes PR #111780.

We made some changed to our downstream code to accommodate the upstream change.
This PR is to modify the EBO static_assert to check against the sizeof(APFloat::Storage) instead of sizeof(detail::IEEEFloat), which seems satisfy both upstream and downstream code.
We also moved the assert to APFloat.cpp from the header file as per discussion in PR #111780

@Ariel-Burton

---

Full diff: https://github.com/llvm/llvm-project/pull/112589.diff

2 Files Affected:

• (modified) llvm/include/llvm/ADT/APFloat.h (+1-3)
• (modified) llvm/lib/Support/APFloat.cpp (+5)

diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index 97547fb577e0ec..85e17efd1fba8e 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -1474,11 +1474,9 @@ class APFloat : public APFloatBase {
   friend APFloat frexp(const APFloat &X, int &Exp, roundingMode RM);
   friend IEEEFloat;
   friend DoubleAPFloat;
+  friend class APFloatEBOChecker;
 };
 
-static_assert(sizeof(APFloat) == sizeof(detail::IEEEFloat),
-              "Empty base class optimization is not performed.");
-
 /// See friend declarations above.
 ///
 /// These additional declarations are required in order to compile LLVM with IBM
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index a33b6c4a6ddc63..f48c82b6afdd89 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -99,6 +99,11 @@ enum class fltNanEncoding {
   NegativeZero,
 };
 
+class APFloatEBOChecker {
+  static_assert(sizeof(APFloat) == sizeof(APFloat::U),
+                "Empty base class optimization is not performed.");
+};
+
 /* Represents floating point arithmetic semantics.  */
 struct fltSemantics {
   /* The largest E such that 2^E is representable; this matches the

[Ariel-Burton]

LGTM

[arsenm]

broke our downstream code as not all the members of APFloat is IEEE.

So I still see no reason to update the upstream code to handle this. Your downstream code is different and should change the assert there

[DanielCChen]

broke our downstream code as not all the members of APFloat is IEEE.

So I still see no reason to update the upstream code to handle this. Your downstream code is different and should change the assert there

Thanks for the comments. I totally understand your point.
In fact, we are in the process of upstreaming our downstream code as @Ariel-Burton stated in the "old" PR.
I think the question is if the change in this PR satisfies the purpose of the original assertion in PR #111641? If not, we can definitely look into it.

[DanielCChen]

Reviewers, could you please take some time to review this PR? If no further comments or objections, I would like to merge it early next week. Thanks in advance!

[nikic]

I would like to keep the assertion as-is. It found a legitimate issue with your downstream changes: That they increase the size of APFloat.

You should drop the assertion downstream for now. When you upstream your changes, we can consider whether we are willing to take that size increase or not, and adjust the assertion accordingly at that time.

[DanielCChen]

I would like to keep the assertion as-is. It found a legitimate issue with your downstream changes: That they increase the size of APFloat.

You should drop the assertion downstream for now. When you upstream your changes, we can consider whether we are willing to take that size increase or not, and adjust the assertion accordingly at that time.

Ok. That is fair. We will bring it back once we are ready to upstream our code in the future.
Thanks again to everyone for reviewing it.