Delete the incorrect assert that assumes size of `APFloat` is the same as `IEEEFloat`. #111780

DanielCChen · 2024-10-10T02:08:14Z

This 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 is to delete the assertion.

@Ariel-Burton

llvmbot · 2024-10-10T02:08:51Z

@llvm/pr-subscribers-llvm-adt

Author: Daniel Chen (DanielCChen)

Changes

This 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 is to delete the assertion.

@Ariel-Burton

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

1 Files Affected:

(modified) llvm/include/llvm/ADT/APFloat.h (-3)

diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index 6f1e24e5da33a9..de11cd93b48478 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -1473,9 +1473,6 @@ class APFloat : public APFloatBase {
   friend DoubleAPFloat;
 };
 
-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

dtcxzyw · 2024-10-10T02:17:36Z

This assertion is to make sure that EBO is performed on APFloat. What is the compiler you use to build llvm?

DanielCChen · 2024-10-10T02:21:06Z

This assertion is to make sure that EBO is performed on APFloat. What is the compiler you use to build llvm?

We are building it on Linux on Power and AIX. This code broke our downstream code we have inside of APFloat that are not IEEE.

dtcxzyw · 2024-10-10T02:26:31Z

Can you please try to compile the following code with your compiler? https://godbolt.org/z/4jqhnjG3E

struct Empty {};
struct FloatImpl1 { float x; };
struct FloatImpl2 { float x; };
struct Float : public Empty {
    union {FloatImpl1 impl1;FloatImpl2 impl2;};
};

static_assert(sizeof(Float) == sizeof(float));

dtcxzyw · 2024-10-10T02:28:24Z

not all the members of APFloat is IEEE.

Oh I see. Do you add a new impl whose size is larger than detail::IEEEFloat?

dtcxzyw · 2024-10-10T02:31:16Z

not all the members of APFloat is IEEE.

Oh I see. Do you add a new impl whose size is larger than detail::IEEEFloat?

If so, at least we can assert sizeof(APFloat) == sizeof(APFloat::Storage).

Ariel-Burton · 2024-10-10T02:32:12Z

This assertion is to make sure that EBO is performed on APFloat. What is the compiler you use to build llvm?

The issue is that APFloat has a union (Storage). Downstream this includes a member / field for another floating point type. We are in the process of upstreaming this type. The representation of this type is larger than IEEE. Note also that you are lucky that the DoubleAPFloat is not larger than IEEEFloat.

It's not a matter of the compiler used to build, but the assumption that no member of the Storage union is larger than IEEEFloat not being true.

Ariel-Burton · 2024-10-10T02:33:07Z

not all the members of APFloat is IEEE.

Oh I see. Do you add a new impl whose size is larger than detail::IEEEFloat?

If so, at least we can assert sizeof(APFloat) == sizeof(APFloat::Storage).

Oh, that might work.

Ariel-Burton · 2024-10-10T03:17:08Z

not all the members of APFloat is IEEE.

Oh I see. Do you add a new impl whose size is larger than detail::IEEEFloat?

If so, at least we can assert sizeof(APFloat) == sizeof(APFloat::Storage).

That's an interesting idea, but needs care to get it to work because if the assert is APFloat it won't compile because APFloat isn't complete until after the closing brace. It can't go outside APFloat because Storage is private. Nor can the assert go in IEEEFloat.

We could add a new friend class APFloarChecker to APFloat and do the check in that new class. Something like this:

lass APFloat {
  union S {
    int i;
    long l;
  } s;
  friend class APFloatChecker;
};

class APFloatChecker {
  static_assert(sizeof(APFloat) == sizeof(APFloat::S));
};

APFloatChecker wouldn't need to go in the .h, where the assertion is evaluated every time the file is processed. It could go in the .cpp, where it will be evaluated once per build.

dtcxzyw · 2024-10-10T03:23:10Z

not all the members of APFloat is IEEE.

Oh I see. Do you add a new impl whose size is larger than detail::IEEEFloat?

If so, at least we can assert sizeof(APFloat) == sizeof(APFloat::Storage).

That's an interesting idea, but needs care to get it to work because if the assert is APFloat it won't compile because APFloat isn't complete until after the closing brace. It can't go outside APFloat because Storage is private. Nor can the assert go in IEEEFloat.

We could add a new friend class APFloarChecker to APFloat and do the check in that new class. Something like this:
lass APFloat {
  union S {
    int i;
    long l;
  } s;
  friend class APFloatChecker;
};

class APFloatChecker {
  static_assert(sizeof(APFloat) == sizeof(APFloat::S));
};
APFloatChecker wouldn't need to go in the .h, where the assertion is evaluated every time the file is processed. It could go in the .cpp, where it will be evaluated once per build.

Make sense to me.

arsenm · 2024-10-10T07:48:56Z

The issue is that APFloat has a union (Storage). Downstream this includes a member / field for another floating point type.

So the assert isn't wrong if you're actually changing the type downstream. If you're changing the type downstream you have to also change the assert downstream

nikic · 2024-10-10T15:05:07Z

Yeah, I don't think we should change the assert upstream. When you add support for your new floating point type upstream, we can decide whether we're willing to increase the APFloat size for it, or you need to reduce the size of your representation (e.g. by indirecting via an allocation, just like DoubleAPFloat does).

DanielCChen · 2024-10-10T19:08:59Z

Thanks for all the comments. We are currently looking into the issue.

…m#98308) ARM uses complex encoding of immediate values using small number of bits. As a result, some values cannot be represented as immediate operands, they need to be synthesized in a register. This change implements legalization of such constants with loading values from constant pool. --------- Co-authored-by: Matt Arsenault <arsenm2@gmail.com>

…#111799)

Factor out and unify common code from InstSimplify and InstCombine that partially guard against cross-lane vector operations into llvm::isNotCrossLaneOperation in ValueTracking. Alive2 proofs for changed tests: https://alive2.llvm.org/ce/z/68H4ka

…)" This reverts commit d6827f6 due to the following CMake configure failure being observed by some: add_custom_target called with invalid target name

…111798) avr-gcc also rejects since these devices has no SRAM. Fixes llvm#96881

…UFB (llvm#112175) If each 128-bit vXi8 lane is shifting the same constant value, we can pre-compute the 8 valid shift results and use PSHUFB to act as a LUT with the shift amount. Fixes llvm#110317

…llvm#111302) Gentoo is planning to introduce a `*t64` suffix for triples that will be used by 32-bit platforms that use 64-bit `time_t`. Add support for parsing and accepting these triples, and while at it make clang automatically enable the necessary glibc feature macros when this suffix is used. An open question is whether we can backport this to LLVM 19.x. After all, adding new triplets to Triple sounds like an ABI change — though I suppose we can minimize the risk of breaking something if we move new enum values to the very end.

llvm#112184) … (llvm#111970)" I was made aware of breakages in Windows/ARM, so reverting while I investigate. This reverts commit f3aebe6.

…ted expressions (llvm#111701) Done by calling clang::runWithSufficientStackSpace(). Added CodeGenModule::runWithSufficientStackSpace() method similar to the one in Sema to provide a single warning when this triggers Fixes: llvm#111699

The profiling code requires GNU extensions as it uses functions such as getpagesize(), fdopen(), etc. The problem manifests when the compiler is built to not default to the extensions mode, e.g. custom config with -std=c2x. CMake didn't support this scenario very well, but it's been fixed by CMP0128. Set the policy to NEW as we now conform to it.

…lvm#110824) Retain SCEVSequentialMinMaxExpr if an operand may trigger UB, e.g. if there is an UDiv operand that may divide by 0 or poison PR: llvm#110824

DanielCChen · 2024-10-16T14:53:24Z

Sorry that I messed up my local branch. I will close this PR and re-post another one shortly.
Please note all these "unexpected" changes are in my local branch. It will not affect the main branch.
Apologies.

DanielCChen requested a review from dtcxzyw October 10, 2024 02:08

DanielCChen self-assigned this Oct 10, 2024

llvmbot added the llvm:adt label Oct 10, 2024

DanielCChen requested a review from arsenm October 10, 2024 02:08

spavloff and others added 13 commits October 16, 2024 10:47

[clang] Move Sema::WarnedStackExhausted from public to private. (llvm…

bfeb7c6

…#111799)

[LangRef] Document that sret only works with void returns (llvm#112167)

4cc57a7

Revert "Fix CMake dependencies on mlir-linalg-ods-yaml-gen (llvm#111973…

e40d6dc

…)" This reverts commit d6827f6 due to the following CMake configure failure being observed by some: add_custom_target called with invalid target name

[clang][Driver][AVR] Reject c/c++ compilation for avr1 devices (llvm#…

78d640e

…111798) avr-gcc also rejects since these devices has no SRAM. Fixes llvm#96881

[X86] LowerShift - lower vXi8 shifts of an uniform constant using PSH…

2906f68

…UFB (llvm#112175) If each 128-bit vXi8 lane is shifting the same constant value, we can pre-compute the 8 valid shift results and use PSHUFB to act as a LUT with the shift amount. Fixes llvm#110317

[X86] vselect-packss.ll - regenerate test checks with vpternlog comments

096417e

Revert "[llvm][ARM] Add Addend Checks for MOVT and MOVW instructions.… (

8989182

llvm#112184) … (llvm#111970)" I was made aware of breakages in Windows/ARM, so reverting while I investigate. This reverts commit f3aebe6.

[SCEV] Retain SCEVSequentialMinMaxExpr if an operand may trigger UB. (l…

2409ead

…lvm#110824) Retain SCEVSequentialMinMaxExpr if an operand may trigger UB, e.g. if there is an UDiv operand that may divide by 0 or poison PR: llvm#110824

DanielCChen requested review from banach-space, dcaballe, MaheshRavishankar, rengolin, JDevlieghere, Endilll, nikic and a team as code owners October 16, 2024 14:49

DanielCChen closed this Oct 16, 2024

DanielCChen mentioned this pull request Oct 16, 2024

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

Closed

ldionne removed request for a team, makslevental, dcci, maksfb, rafaelauler, nicolasvasilache, Endilll, dtcxzyw, dcaballe, Moxinilian, PeimingLiu, aartbik, ayermolo, DeinAlptraum and yinying-lisa-li October 16, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete the incorrect assert that assumes size of `APFloat` is the same as `IEEEFloat`. #111780

Delete the incorrect assert that assumes size of `APFloat` is the same as `IEEEFloat`. #111780

DanielCChen commented Oct 10, 2024

llvmbot commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

DanielCChen commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

Ariel-Burton commented Oct 10, 2024

Ariel-Burton commented Oct 10, 2024 •

edited

Loading

Ariel-Burton commented Oct 10, 2024 •

edited

Loading

dtcxzyw commented Oct 10, 2024

arsenm commented Oct 10, 2024

nikic commented Oct 10, 2024

DanielCChen commented Oct 10, 2024

DanielCChen commented Oct 16, 2024

Delete the incorrect assert that assumes size of APFloat is the same as IEEEFloat. #111780

Delete the incorrect assert that assumes size of APFloat is the same as IEEEFloat. #111780

Conversation

DanielCChen commented Oct 10, 2024

llvmbot commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

DanielCChen commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

dtcxzyw commented Oct 10, 2024

Ariel-Burton commented Oct 10, 2024

Ariel-Burton commented Oct 10, 2024 • edited Loading

Ariel-Burton commented Oct 10, 2024 • edited Loading

dtcxzyw commented Oct 10, 2024

arsenm commented Oct 10, 2024

nikic commented Oct 10, 2024

DanielCChen commented Oct 10, 2024

DanielCChen commented Oct 16, 2024

Delete the incorrect assert that assumes size of `APFloat` is the same as `IEEEFloat`. #111780

Delete the incorrect assert that assumes size of `APFloat` is the same as `IEEEFloat`. #111780

Ariel-Burton commented Oct 10, 2024 •

edited

Loading

Ariel-Burton commented Oct 10, 2024 •

edited

Loading