[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
https://github.com/urnathan created https://github.com/llvm/llvm-project/pull/87090 This follows on from PR65742. As I suspected clipTailPadding's functionality can be moved into accumulateBitfields; it has sufficient information now. It turns out that clipTailPadding was undoing some of the work just added to accumulateBitfields. Specifically, on a strict-alignment LP64 machine, accumulateBitfields could determine that a 24-bit access unit was the best unit to use (loading 3 separate bytes being better than 4). But if those 3 bytes were followed by a padding byte, clipTailPadding would not make the storage a 3-byte array, and the access unit would be an (unaligned) i32. Boo! The new testcase shows this -- it's written to work on ILP32 and LP64, but the common case I think would be on LP64 where a pointer follows the bitfields, and there's 4 bytes of alignment padding between. The fine-grained-bitfield accesses needed adjusting, to preserve the existing behaviour of extending them into useable tail padding. You'll see we now check this later -- allowing it to grow, but preventing accumulation of a subsequent span. I think that's in keeping with the desired behaviour of that option. >From 4d9861a9ca23d8de7b33c4b830b5c6f78e8a3768 Mon Sep 17 00:00:00 2001 From: Nathan Sidwell Date: Sun, 24 Mar 2024 09:37:46 -0400 Subject: [PATCH 1/2] Move bitfield clipping into bitfield accumulation --- clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 63 - clang/test/CodeGen/bitfield-access-unit.c | 18 ++ 2 files changed, 55 insertions(+), 26 deletions(-) diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp index e32023aeac1e6f..5b9045f661e048 100644 --- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp +++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp @@ -41,10 +41,11 @@ namespace { /// contains enough information to determine where the runs break. Microsoft /// and Itanium follow different rules and use different codepaths. /// * It is desired that, when possible, bitfields use the appropriate iN type -/// when lowered to llvm types. For example unsigned x : 24 gets lowered to +/// when lowered to llvm types. For example unsigned x : 24 gets lowered to /// i24. This isn't always possible because i24 has storage size of 32 bit -/// and if it is possible to use that extra byte of padding we must use -/// [i8 x 3] instead of i24. The function clipTailPadding does this. +/// and if it is possible to use that extra byte of padding we must use [i8 x +/// 3] instead of i24. This is computed when accumulating bitfields in +/// accumulateBitfields. /// C++ examples that require clipping: /// struct { int a : 24; char b; }; // a must be clipped, b goes at offset 3 /// struct A { int a : 24; ~A(); }; // a must be clipped because: @@ -197,9 +198,7 @@ struct CGRecordLowering { /// not the primary vbase of some base class. bool hasOwnStorage(const CXXRecordDecl *Decl, const CXXRecordDecl *Query); void calculateZeroInit(); - /// Lowers bitfield storage types to I8 arrays for bitfields with tail - /// padding that is or can potentially be used. - void clipTailPadding(); + void checkTailPadding(); /// Determines if we need a packed llvm struct. void determinePacked(bool NVBaseType); /// Inserts padding everywhere it's needed. @@ -302,7 +301,7 @@ void CGRecordLowering::lower(bool NVBaseType) { } llvm::stable_sort(Members); Members.push_back(StorageInfo(Size, getIntNType(8))); - clipTailPadding(); + checkTailPadding(); determinePacked(NVBaseType); insertPadding(); Members.pop_back(); @@ -521,6 +520,7 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // available padding characters. RecordDecl::field_iterator BestEnd = Begin; CharUnits BestEndOffset; + bool BestClipped; // Whether the representation must be in a byte array. for (;;) { // AtAlignedBoundary is true iff Field is the (potential) start of a new @@ -583,10 +583,9 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // this is the best seen so far. BestEnd = Field; BestEndOffset = BeginOffset + AccessSize; -if (Types.getCodeGenOpts().FineGrainedBitfieldAccesses) - // Fine-grained access, so no merging of spans. - InstallBest = true; -else if (!BitSizeSinceBegin) +// Assume clipped until proven not below. +BestClipped = true; +if (!BitSizeSinceBegin) // A zero-sized initial span -- this will install nothing and reset // for another. InstallBest = true; @@ -614,6 +613,12 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // The access unit is not at a naturally aligned offset within the // structure. InstallBest = true; + + if (InstallBest && BestEnd == Fi
[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
llvmbot wrote: @llvm/pr-subscribers-clang Author: Nathan Sidwell (urnathan) Changes This follows on from PR65742. As I suspected clipTailPadding's functionality can be moved into accumulateBitfields; it has sufficient information now. It turns out that clipTailPadding was undoing some of the work just added to accumulateBitfields. Specifically, on a strict-alignment LP64 machine, accumulateBitfields could determine that a 24-bit access unit was the best unit to use (loading 3 separate bytes being better than 4). But if those 3 bytes were followed by a padding byte, clipTailPadding would not make the storage a 3-byte array, and the access unit would be an (unaligned) i32. Boo! The new testcase shows this -- it's written to work on ILP32 and LP64, but the common case I think would be on LP64 where a pointer follows the bitfields, and there's 4 bytes of alignment padding between. The fine-grained-bitfield accesses needed adjusting, to preserve the existing behaviour of extending them into useable tail padding. You'll see we now check this later -- allowing it to grow, but preventing accumulation of a subsequent span. I think that's in keeping with the desired behaviour of that option. --- Full diff: https://github.com/llvm/llvm-project/pull/87090.diff 2 Files Affected: - (modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+43-35) - (modified) clang/test/CodeGen/bitfield-access-unit.c (+18) ``diff diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp index e32023aeac1e6f..acf1746294ef73 100644 --- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp +++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp @@ -41,10 +41,11 @@ namespace { /// contains enough information to determine where the runs break. Microsoft /// and Itanium follow different rules and use different codepaths. /// * It is desired that, when possible, bitfields use the appropriate iN type -/// when lowered to llvm types. For example unsigned x : 24 gets lowered to +/// when lowered to llvm types. For example unsigned x : 24 gets lowered to /// i24. This isn't always possible because i24 has storage size of 32 bit -/// and if it is possible to use that extra byte of padding we must use -/// [i8 x 3] instead of i24. The function clipTailPadding does this. +/// and if it is possible to use that extra byte of padding we must use [i8 x +/// 3] instead of i24. This is computed when accumulating bitfields in +/// accumulateBitfields. /// C++ examples that require clipping: /// struct { int a : 24; char b; }; // a must be clipped, b goes at offset 3 /// struct A { int a : 24; ~A(); }; // a must be clipped because: @@ -197,9 +198,7 @@ struct CGRecordLowering { /// not the primary vbase of some base class. bool hasOwnStorage(const CXXRecordDecl *Decl, const CXXRecordDecl *Query); void calculateZeroInit(); - /// Lowers bitfield storage types to I8 arrays for bitfields with tail - /// padding that is or can potentially be used. - void clipTailPadding(); + void checkTailPadding(); /// Determines if we need a packed llvm struct. void determinePacked(bool NVBaseType); /// Inserts padding everywhere it's needed. @@ -302,7 +301,7 @@ void CGRecordLowering::lower(bool NVBaseType) { } llvm::stable_sort(Members); Members.push_back(StorageInfo(Size, getIntNType(8))); - clipTailPadding(); + checkTailPadding(); determinePacked(NVBaseType); insertPadding(); Members.pop_back(); @@ -521,6 +520,7 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // available padding characters. RecordDecl::field_iterator BestEnd = Begin; CharUnits BestEndOffset; + bool BestClipped; // Whether the representation must be in a byte array. for (;;) { // AtAlignedBoundary is true iff Field is the (potential) start of a new @@ -583,10 +583,9 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // this is the best seen so far. BestEnd = Field; BestEndOffset = BeginOffset + AccessSize; -if (Types.getCodeGenOpts().FineGrainedBitfieldAccesses) - // Fine-grained access, so no merging of spans. - InstallBest = true; -else if (!BitSizeSinceBegin) +// Assume clipped until proven not below. +BestClipped = true; +if (!BitSizeSinceBegin) // A zero-sized initial span -- this will install nothing and reset // for another. InstallBest = true; @@ -614,6 +613,12 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // The access unit is not at a naturally aligned offset within the // structure. InstallBest = true; + + if (InstallBest && BestEnd == Field) +// We're installing the first span, who's clipping was +// conservatively presumed above. Compute it correctly. +
[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
urnathan wrote: Sigh, clipTailPadding is still needed, because of no_unique_address and empty base placement. Will update. https://github.com/llvm/llvm-project/pull/87090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
https://github.com/urnathan converted_to_draft https://github.com/llvm/llvm-project/pull/87090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
urnathan wrote: @rjmccall can you take a look at this? as the description says, this turns out to not be NFC, but important on (probably only) LP64 machines. https://github.com/llvm/llvm-project/pull/87090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
https://github.com/rjmccall approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/87090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move tailclipping to bitfield allocation (PR #87090)
https://github.com/urnathan closed https://github.com/llvm/llvm-project/pull/87090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits