dblaikie added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
// The align if the field is not packed. This is to check if the attribute
// was unnecessary (-Wpacked).
CharUnits UnpackedFieldAlign =
!DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
CharUnits UnpackedFieldOffset = FieldOffset;
CharUnits OriginalFieldAlign = UnpackedFieldAlign;
----------------
dblaikie wrote:
> rsmith wrote:
> > It seems a little wasteful and error-prone that we're now computing the
> > actual alignment, the alignment if the field were not packed, and the
> > alignment if the field were packed. Is there any way we can reduce this
> > down to computing just the alignment if the field were packed plus the
> > alignment if the field were not packed, then picking one of those two as
> > the actual field alignment? Or does that end up being messier?
> I had a go at that refactor - we can't pull the `FieldPacked` computation
> lower (it'd be great if it could move down to after the packed/unpacked
> computation, so it was clear that those values were computed independently of
> the `FieldPacked` value, and that `FieldPacked` just determined which one to
> pick) because of the `alignedAttrCanDecreaseAIXAlignment`, by the looks of it.
>
> And also the AIX alignment stuff seems to do some weird things around the
> preferred alignment that caused the carefully constructed 3 `if`s below
> (`!FieldPacked`, `DefaultsToAIXPowerAlignment`, and `FieldPacked`) which I
> spent more time than I'd like to admit figuring out why anything
> else/less/more streamlined was inadequate.
>
> But I also don't understand why `DefaultsToAIXAlignment` causes the `AlignTo`
> value to be the `PreferredAlign`, but the `FieldAlign` stays as it is? (like
> why doesn't `DefaultsToAIXPowerAlignment` cause `FieldAlign` to /be/
> `PreferredAlign` - I think that'd simplify things, but tests (maybe the tests
> are incorrect/) seemed to break when I tried that) - I would've thought not
> doing that (as the code currently doesn't) would cause problems for the
> `UnadjustedAlignment`, `UpdateAlignment`, and `warn_unaligned_access` issues
> later on that depend on `FieldAlign`, but feel like they should probably
> depend on the alignment that actually got used (the `PreferredAlign`)
> instead? It's pretty confusing to me, so... yeah.
Ping on this discussion. @rsmith
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118511/new/
https://reviews.llvm.org/D118511
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
