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

[Timur Iskhodzhanov]

ping

2013/8/9 Timur Iskhodzhanov <timurrrr at google.com>:
> Ping
>
> 06 авг. 2013 г. 20:32 пользователь "Timur Iskhodzhanov"
> <timurrrr at google.com> написал:
>
>> 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.