[PATCH] D77592: [NFC][CodeGen] Add enum for selecting the layout of components in the vtable

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D77592#1987677 <https://reviews.llvm.org/D77592#1987677>, @leonardchan wrote:

> In D77592#1985740 <https://reviews.llvm.org/D77592#1985740>, @rjmccall wrote:
>
> > I'm not sure if the AST-level v-table layout abstraction really cares about these differences.  I don't think it vends byte offsets into the v-table, just slot indices (i.e. word offsets).
>
>
> The primary reason for why I added it in `AST` is because down the line, to support the changes to the RTTI component, I'll need to edit `VCallAndVBaseOffsetBuilder::getCurrentOffsetOffset()`:
>
>   CharUnits VCallAndVBaseOffsetBuilder::getCurrentOffsetOffset() const {
>     // OffsetIndex is the index of this vcall or vbase offset, relative to the
>     // vtable address point. (We subtract 3 to account for the information just
>     // above the address point, the RTTI info, the offset to top, and the
>     // vcall offset itself).
>     int64_t OffsetIndex = -(int64_t)(3 + Components.size());
>  
>     CharUnits PointerWidth =
>       Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
>     CharUnits OffsetOffset = PointerWidth * OffsetIndex;
>     return OffsetOffset;
>   }
>
>
> Right now, it assumes that the offset is 3 pointer widths below the vtable address point, but since we want to make the RTTI component a 32-bit int, we'll need to add an extra 4 `CharUnits` to the result.


Okay, sure, if there are already offsets in bytes already in the AST-level layout, I agree that we should be able to compute all of the byte offsets at that level.

How are you planning to handle alignments here?  Currently alignment doesn't affect the layout because all the entries are uniformly pointer-sized.  If you're planning to make the offset-to-top and vcall offsets 64-bit on 64-bit architectures, you're going to either have interior padding or those offsets might not be naturally-aligned.

Personally, I would just use 32-bit offsets unless you really think you need to support types with base adjustments more than ±2GB.  And that has the nice impact of making all the offsets within the v-table uniformly scaled again, just by 4 bytes instead of the pointer size.

Different topic: I'm still not sure why the places you've added TODOs here are actually places you'd need to modify.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77592