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

[Timur Iskhodzhanov]

Thanks for the comments!
Please take a look at the updated patch.

2013/8/5 John McCall <rjmccall at gmail.com>:
> 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?

Agreed, done.

> +  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.

Hm, I think I was able to put the correct value into alloca doing the
adjustment in CodeGenFunction::EmitFunctionProlog(), please take a
look if this approach is good enough?

// At least this produces great LLVM code now!

> +  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.

Good point - done!

> 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.)

Done

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

Yep, that's 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.

Done

> +    /// 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.

I assume you're referring to that "interesting" case when vbases got
reordered and the "this" parameter was expected to point past the
complete object?
Yeah, I was considering using i8* from the beginning, but didn't
completely come up with this special case yet...
Done, now getThisArgumentTypeForMethod() is allowed to return 0 if it
wants to indicate that we should use i8*.

I kinda agree that calculating ThisType is a somewhat redundant step
even if there is no virtual inheritance; I've put a FIXME for that
just in case.

I think my current solution (passing RD=0) is somewhat sloppy, but
can't come up with a better alternative. Do you have any suggestions
there?

[I also considered making arrangeCXXMethodType() take MD rather than
RD and making a CanQualType callback rather than RD-returning, but it
turned out arrangeCXXMethodType() should actually work with "unknown
C++ non-static member function of the given abstract type".]

> John.