Thanks for the comments! Sorry I missed some before, I blame Phabricator.
New patch coming up.
================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1972
@@ +1971,3 @@
+ CGF.FinishFunction();
+ CGM.setFunctionLinkage(MD, ThunkFn); // XXX: Do we need this?
+
----------------
Hans Wennborg wrote:
> Reid Kleckner wrote:
> > Timur Iskhodzhanov wrote:
> > > Reid Kleckner wrote:
> > > > Timur Iskhodzhanov wrote:
> > > > > Isn't this copying the function's linkage over to the thunk?
> > > > > Don't you get "symbol already defined" if you link two TUs generating
> > > > > the same thunk together?
> > > > These should be merged like inline functions. They should be
> > > > linkonce_odr for externally visible types and internal for internal
> > > > types. We probably have logic for that somewhere.
> > > I don't see this in the lit test expectations:
> > >
> > > define x86_thiscallcc void @"\01??_9C@@$B7AE"
> > Yes, I agree with you, the logic needs to change. :) I'm just adding that
> > if the type is internal than we can make the thunk internal too, which is
> > slightly better for the optimizers. We can probably reuse some existing
> > code for this.
> Yeah, I think the answer to "Do we need this?" is "no" :)
>
> That means they get linkonce_odr linkage, which should work right?
> I don't see this in the lit test expectations:
>
> define x86_thiscallcc void @"\01??_9C@@$B7AE"
Right, I failed to update the test to check for the linkage :(
================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1972
@@ +1971,3 @@
+ CGF.FinishFunction();
+ CGM.setFunctionLinkage(MD, ThunkFn); // XXX: Do we need this?
+
----------------
Hans Wennborg wrote:
> Hans Wennborg wrote:
> > Reid Kleckner wrote:
> > > Timur Iskhodzhanov wrote:
> > > > Reid Kleckner wrote:
> > > > > Timur Iskhodzhanov wrote:
> > > > > > Isn't this copying the function's linkage over to the thunk?
> > > > > > Don't you get "symbol already defined" if you link two TUs
> > > > > > generating the same thunk together?
> > > > > These should be merged like inline functions. They should be
> > > > > linkonce_odr for externally visible types and internal for internal
> > > > > types. We probably have logic for that somewhere.
> > > > I don't see this in the lit test expectations:
> > > >
> > > > define x86_thiscallcc void @"\01??_9C@@$B7AE"
> > > Yes, I agree with you, the logic needs to change. :) I'm just adding
> > > that if the type is internal than we can make the thunk internal too,
> > > which is slightly better for the optimizers. We can probably reuse some
> > > existing code for this.
> > Yeah, I think the answer to "Do we need this?" is "no" :)
> >
> > That means they get linkonce_odr linkage, which should work right?
> > I don't see this in the lit test expectations:
> >
> > define x86_thiscallcc void @"\01??_9C@@$B7AE"
>
> Right, I failed to update the test to check for the linkage :(
> Yes, I agree with you, the logic needs to change. :) I'm just adding that if
> the type is internal than we can > make the thunk internal too, which is
> slightly better for the optimizers. We can probably reuse some existing code
> for this.
Doing this.
================
Comment at: lib/CodeGen/CGVTables.cpp:243
@@ +242,3 @@
+ const CGFunctionInfo &FnInfo) {
+ assert(!CurGD.getDecl() && "CurGD was already set!");
+ CurGD = GD;
----------------
Timur Iskhodzhanov wrote:
> How about "Please use a new CGF for this thunk" instead?
> I think such a string would make it clearer what one is doing wrong if this
> fires.
Done.
================
Comment at: lib/CodeGen/CodeGenFunction.h:1159
@@ +1158,3 @@
+ void EmitCallAndReturnForThunk(GlobalDecl GD, llvm::Value *Callee,
+ llvm::Value *AdjustedThisPtr,
+ const ThunkInfo *Thunk);
----------------
Timur Iskhodzhanov wrote:
> Is there any strong reason why "this" adjustment can't be performed by this
> function?
Yes, the vcall thunks shouldn't do this-adjustment.
================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1005
@@ +1004,3 @@
+ CGF.EmitCallAndReturnForThunk(MD, Callee, This, 0);
+
+ CGF.FinishFunction();
----------------
Timur Iskhodzhanov wrote:
> Just curious: there's an AutoreleaseResult assignment before FinishFunction
> when emitting standard thunks.
> Have you checked whether we need that for vmemfunptr thunks?
Hmm, we probably should actually.
================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1434
@@ +1433,3 @@
+ FirstField = llvm::Constant::getNullValue(CGM.VoidPtrTy);
+ } else if (CGM.getMicrosoftVTableContext()
+ .getMethodVFTableLocation(MD)
----------------
Reid Kleckner wrote:
> Timur Iskhodzhanov wrote:
> > how about
> >
> > } else {
> > ... &ML = CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD);
> > if (ML.VBase) {
> > ...
> > } else {
> > ...
> > }
> > }
> >
> > ?
> +1
The nesting is starting to get deep here :) Pulling out the definition of ML
seems like a good idea though, I'll do that.
================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1446
@@ +1445,3 @@
+ bool Is64Bit = getContext().getTargetInfo().getPointerWidth(0) == 64;
+ int OffsetInVFTable = ML.Index * (Is64Bit ? 8 : 4);
+ llvm::raw_svector_ostream Out(ThunkName);
----------------
Timur Iskhodzhanov wrote:
> Earlier comment by Reid says:
>
> IMO the right way to write this is something like:
>
> ML.Index * getContext().getTypeSizeInChars(getContext().VoidPtrTy)
>
Done. Sorry I missed that comment before.
http://llvm-reviews.chandlerc.com/D2104
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
