[PATCH] D51905: Front-end of the implementation of the interleaving algorithm

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Driver/CC1Options.td:356-357
+    HelpText<"Enable VTable interleaving">;
+def disable_vtable_interleaving : Flag<["-"], "disable-vtable-interleaving">,
+    HelpText<"Disable VTable interleaving">;
 //===----------------------------------------------------------------------===//
----------------
We usually only expose the non-default flag in `-cc1`, so that there are no ordering concerns and we can determine whether a feature is enabled with just `hasArg`. Also, `-fvtable-interleaving` would seem like a more natural flag name to me.


================
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:274
+  /// the interleaving layout is decided.
+  bool EnableVTableInterleaving;
+
----------------
`Enable` here seems redundant. Is thee a reason to declare this explicitly rather than adding it to CodeGenOptions.def?


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:973-976
+    // The name of a offset global variable has the format "__[type
+    // id]$[offset]".
+    std::string Name =
+        "__" + RD->getNameAsString() + "$" + std::to_string(Offset);
----------------
The `__`s here are atypical. If you want a name that can't collide with a name generated by the frontend, putting a period in it is the usual strategy. Also, a name prefix that indicates what this global is for would make the IR more readable and make this less likely to collide with other things. Maybe `clang.vtable.index.<CLASS>.<OFFSET>` or similar?

Also, there's no guarantee that `RD->getNameAsString()` produces something unique within the translation unit, so you'll need to make sure that name collisions are handled somehow (I *think* `GlobalVariable` already deals with this for you, but I'm not sure... however, I think its strategy is to add a number to the end of the mangling, which will lead to some quite peculiar results given that you already have a number there). It might be easier to base the name of this symbol on the mangled name of the class; then you could give the global variable the same linkage as the class, which might save your interleaving pass a little bit of work later on as the duplicates will already have been combined -- but that's up to you, and I don't have enough of the big picture here to give you advice.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:987
+  }
+  return llvm::ConstantExpr::getPtrToInt(OffsetGV, OffsetType);
+}
----------------
Representing this offset as a `ptrtoint` on a global variable seems really strange. Is there really no better way to model this in IR? (I mean, if not, then carry on as you are, but this seems like a hack.)


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1767
 
+  bool InBounds = shouldInterleaveVTables(VTableClass) ? false : true;
   return llvm::ConstantExpr::getGetElementPtr(VTable->getValueType(), VTable,
----------------
zhaomo wrote:
> pcc wrote:
> > Remind me why this needs to be here? (And the explanation needs to be in a comment.)
> The calculation of address point is essentially base_vtable_addr + offset, where offset is from the indices of gep.
> In the interleaving pass, we replace base_vtable_addr with (addr_point_in_interleaved_layout - offset).
> 
> The LLVM language reference says that the base address of a inbounds gep must be an in bound address of the object. The new base address addr_point_in_interleaved_layout - offset, however, may not be an in bound address.
I still find the need for this confusing. Suppose we have:

```
struct A { virtual void f(); };
struct B { virtual void g(); virtual void h(); };
struct C : A, B {};
```

such that `C` has a naive vtable containing 7 elements {{offset, typeid, &A::f}, {offset, typeid, &B::f, &B::h}}, with address point indexes (0, 2) and (1, 2). Here we emit a `gep inbounds @vtable, 0, 1, 2` to get the B-in-C vtable. Now, if this were instead emitted as `gep inbounds (cast @vtable to i8*), 24`, and the interleaving process were replacing @vtable with a non-`inbounds` GEP on its interleaved ubervtable, I could understand the need to drop `inbounds` here -- because the replacement for @vtable might be before the start of the ubervtable. But based on my reading of the algorithm description, that's not what happens: first we split `@vtable` into multiple individual vtables, and that splitting process presumably must identify these GEPs to figure out which slice of the vtable they need. That presumably either removes this GEP entirely, or leaves this being a correct and trivial `inbounds` GEP on a split-out piece of the vtable. (I'm imagining we effectively rewrite `gep inbounds @vtable, 0, 1, 2` as `@vtable-slice-2` as part of the splitting.) And then after interleaving, I'm imagining we replace all uses of `@vtable-slice-2` with the appropriate address point in the ubervtable, which is again an `inbounds` GEP (but that would be a new GEP, unrelated to this one).

That is, if you need this change, it seems like that indicates a bug in the vtable splitter. But maybe I'm misunderstanding how this all fits together.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1832
+  auto *RecordDecl = MethodDecl->getParent();
+  llvm::Value *VTable = CGF.GetVTablePtr(This, CGM.Int8PtrTy, RecordDecl);
 
----------------
Do you really need to change this to load the vptr at the "wrong" type and then cast it? (Could the cast, if necessary at all, live in `EmitVTableTypeCheckedLoad` instead?)


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:3618-3622
+  if (!CXXABI.shouldInterleaveVTables(RD)) {
+    uint64_t UpperBits = (uint64_t)Offset << 8;
+    uint64_t OffsetFlags = UpperBits | LowerBits;
+    return llvm::ConstantInt::get(OffsetFlagsTy, OffsetFlags);
+  }
----------------
Shouldn't you also do this if the base is non-virtual? In that case,`Offset` is an offset within the class layout, not an index into the vtable.


https://reviews.llvm.org/D51905