[PATCH] D62362: [COFF] De-virtualize Chunk and SectionChunk

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 2 inline comments as done.
rnk added inline comments.


================
Comment at: lld/COFF/Chunks.h:355
+
+inline size_t Chunk::getSize() const {
+  if (isa<SectionChunk>(this))
----------------
aganea wrote:
> This is back to the future! C with manual vtables :) 64-bit pointers are a big problem for space. It'd be nice if we could optionally have 32-bit or even 16-bit pointers (yes it opens the door to other issues, but that could be catched with sanitizers or assertions). In many cases we pool objects of a given type or a given class hierarchy, so they are quite close to each other - having an option to copy the vtable close to the allocated pool would be nice to alleviate for issues like this. A 16-bit index would be more that enough in that case, in place of the 64-bit vtable.
Definitely, back to the future. :) I actually did this before for the LLVM IR Value / Instruction class hierarchy:
https://github.com/llvm/llvm-project/blob/308e82ecebeef1342004637db9fdf11567a299b3/llvm/lib/IR/Value.cpp#L98
https://github.com/llvm/llvm-project/blob/308e82ecebeef1342004637db9fdf11567a299b3/llvm/lib/IR/Instruction.cpp#L654
There we have an almost completely closed type hierarchy, so all cases are enumerated in a switch, but here in LLD, some Chunks are defined in files, and we'd first have to give enums to them all. We surely don't need more than 256 Chunk types, so a byte is enough.


================
Comment at: lld/COFF/Chunks.h:377
+
+inline bool Chunk::isHotPatchable() const {
+  if (isa<SectionChunk>(this))
----------------
aganea wrote:
> Since you're doing this, I'm just wondering if the behavior for `isHotPatchable` shouldn't all be here? I find there's value having all related code together. Maybe there are other cases for the other "virtual-ish" functions below.
> ```
> inline bool Chunk::isHotPatchable() const {
>   if (ChunkKind == SectionKind)
>     return File->HotPatchable;
>   return ChunkKind  == ImportThunk; // add ImportThunk kind
> }
> ```
> 
> 
I think the way to go is to make this change, and then tackle the remaining virtual methods one by one. This patch replaces the `hasData()` virtual method with the `HasData` bit, and there are a number of other methods here that could probably use the same treatment.

Initially I tried to lift section Characteristics up into Chunk in the first version of this patch, but it didn't work out, and I backed it out. I put the P2Align data in to Characteristics and had all the Chunk subtypes simply set their Characteristics in the constructor, but I needed another byte for the ChunkKind, or at least a bit to indicate SectionChunk vs. NonSectionChunk. I couldn't get the data packed in right without doing more unrelated rearrangements, so I left it for later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62362