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

[Timur Iskhodzhanov]

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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130809/41824e15/attachment.html>