[PATCH] D87029: [AIX] Implement AIX special bitfield related alignment rules

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16447
+
+    bool AIXBitfieldViolation = false;
+    if (const BuiltinType *BTy = FieldTy.getTypePtr()->getAs<BuiltinType>()) {
----------------
sfertile wrote:
> Xiangling_L wrote:
> > sfertile wrote:
> > > Can  this change can be split out into its own patch? If it can i would suggest doing so.
> > I was expecting our buildbot can pick up all bitfield related changes at one time. Also if we split this out, that means we either need to wait for this second part to land first or need to add more LIT to oversized long long to the first part, which then needs to be removed whenever this second part land. It seems we are complicating the patch. Can you give me your rationale about why we want to split out this part?
> > I was expecting our buildbot can pick up all bitfield related changes at one time.
> IIUC `clang/test/Layout/aix-oversized-bitfield.cpp` works with just this change and isn't dependent on D87702. Its disjoint from the other changes in this patch, and packaging it into a commit with unrelated changes even if they are on the same theme is not beneficial. Its better to have those run through the build bot (or be bisectable) as distinct changes.
> 
> > Also if we split this out, that means we either need to wait for this second part to land first or need to add more LIT to oversized long long to the first part, which then needs to be removed whenever this second part land.  It seems we are complicating the patch.
> 
> I don't understand why it would need to wait or require extra testing to be added. Its a diagnostic and your lit test shows the error for 32-bit (where we want it emitted)  and expected layout for 64-bit. The whole point of splitting it out is that its simple,does exactly one thing, is testable on its own,  and we don't need the context of the other changes packaged with it to properly review it. I am asking to split it out because I see it as making this easier to review and commit.
Sure, I will split this patch into two as you suggested. By `either need to wait for this second part to land first or need to add more LIT `, I thought we would like to also add test coverage and later remove it for oversize bitfield. Since `StorageUnitSize > 32 && Context.getTargetInfo().getTriple().isArch32Bit()` does affect how oversize bitfield get laid out on AIX. But I guess it's more convenient to just split this patch as you suggested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87029/new/

https://reviews.llvm.org/D87029