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:
> 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
@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
