[PATCH] Abstract out virtual calls and virtual function prologue code generation

[John McCall]

On Aug 2, 2013, at 2:34 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>  John,
>  Any comments on this?

+llvm::Value *ItaniumCXXABI::BuildVirtualCall(CodeGenFunction &CGF,
+                                             GlobalDecl GD, llvm::Value *This,
+                                             llvm::Type *Ty) {

I know you’re just using an existing name here, but the function is still
misnamed: it doesn’t actually build the call, it just gets the virtual function
pointer.  So, maybe getVirtualFunctionPointer?

+  llvm::Value *result =
+      CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(getThisDecl(CGF)), "this");
+
+  const CXXMethodDecl *MD = cast<CXXMethodDecl>(CGF.CurGD.getDecl());
+  if (MD->isVirtual()) {
+    result = CGM.getCXXABI()
+        .adjustThisParameterInVirtualFunctionPrologue(CGF, CGF.CurGD, result);
+  }
+
+  getThisValue(CGF) = result;

The alloca should hold the correct value; that’s important for e.g. debug info.
It’d be nice to do this during argument emission, but if necessary you can just
store the adjusted value back into the alloca.

+  unsigned AS = cast<llvm::PointerType>(This->getType())->getAddressSpace();
+  llvm::Type *charPtrTy = CGF.Int8Ty->getPointerTo(AS),
+             *thisTy = This->getType();
+  if (ML.VBase) {
+    This = CGF.Builder.CreateBitCast(This, charPtrTy);
+    const ASTRecordLayout &DerivedLayout =
+        CGF.getContext().getASTRecordLayout(MD->getParent());
+    CharUnits VBaseOffset = DerivedLayout.getVBaseClassOffset(ML.VBase);
+    This = CGF.Builder.CreateConstGEP1_64(This, -VBaseOffset.getQuantity());
+  }
+
+  CharUnits StaticOffset = ML.VFTableOffset;
+  if (!StaticOffset.isZero()) {
+    assert(StaticOffset.isPositive());
+    This = CGF.Builder.CreateBitCast(This, charPtrTy);
+    This = CGF.Builder.CreateConstGEP1_64(This, -StaticOffset.getQuantity());
+  }

Just accumulate the constant offset locally and do one gep.

You should add a quick explanation of the algorithm here, covering why using
the vbase offset within the defining class is the right thing to do.  (It doesn’t
need to be too long — just a sentence or two, then just refer to the comments
in the vf-table layout code for a more complete explanation.)

Also, this will eventually need vtordisp logic, right?  Or is that done with thunks?

+    /// VBase - If nonzero, holds the virtual base which holds this method
+    /// in its vftable.
+    const CXXRecordDecl *VBase;

“nonnull” instead of “nonzero”.

Also, the exact definition is that this is the virtual base subobject which contains
the vf-table that the method definition is adjusted to.

+    /// Stores the method's "this" implicit argument type.
+    const CXXRecordDecl *ThisType;

You seem to be going through a lot of effort to propagate a type around that,
frankly, isn’t even accurate when virtual bases come into play.  Consider just
passing it as an i8*, at least in the vbase case.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130805/8438db12/attachment.html>