Generating vptr assumption loads for better devirtualization.
Some first round review comments.
Sentences should start with a capital letter and end with a period.
Variables should be capitalized.
The return should be on it's own line, please clang-format this.
This kind of initialization is not very common in LLVM.
Capital letter and period missing here.
Would it make more sense for these to live in CGClass ?
I'm not sure what this function is supposed to compute. Needed for what?
In the MS ABI, derived classes never share vtables with bases. Why do you need to do anything other than check that the most derived class doesn't have the __declspec(novtable) ?
|387–391 ↗||(On Diff #31571)|
Please commit this cleanup separately.
Please add documentation comments for these.
Typo Initilize -> Initialize
Comment is out of date.
Please start comments with a capital letter and end them with a full stop.
Is this done? :)
Looks like another formatting/tab issue here.
Looks like the same formatting/tab as above.
This is what I've done when using clang-format, maybe it will fix the issues you've had:
(Just don't include the less than and greater around the FileToFormat.)
I think this comment should be a little more verbose. How about this:
// Generate vtable assumptions if we are calling dynamic-class's ctor, except when doing so as part of a derived class's ctor's base-class initialization. Doing so in this latter case would be useless, because the vtable is about to be overwritten by the derived class's vtable.
Will these IR values still have names even when compiling an -Asserts (NDEBUG) build? If not, you'll need to have better pattern matching.
Also, please match the load instructions that calculate %vtable (and the call to the ctor so that we know that the call to the assumption comes after it).
I think it would be nicer to put the checks for each function adjacent to the function definitions themselves. So put these CHECK lines right next to fooA().
Don't match the TBAA metadata (leaving off everything after the comma before !tbaa should be fine).
The main point of not calling this function, is because it is useless to have assumption loads inside constructor, when they are generated also outside of ctor. I guess You could have case when You are not overriding base vptr, and You are calling base ctor from dynamic class
It might also be worth including in the comment that it is not correct to call EmitVTableAssumptionLoads here, because it assumes the object's vptr points to the complete object vtable; during a constructor call it will probably have a construction vtable instead.
EmitVTableAssumptionLoads assumes that the This pointer points to an object whose most-derived type is ClassDecl, and it assumes that the object is not under construction. Both of those assumptions can be violated for a base class constructor call, and can result in it emitting incorrect assumptions (because virtual bases may be at different offsets and because the vptr of an object under construction may be different from the corresponding vptr of a fully-constructed object).
Mostly LGTM. Are you going to emit assumptions for vbptrs in a separate patch?
In contrast, this does not need to be as general as it is, and making it this general is actively harmful; just pass the vtable class.
Also, I suggest calling it "doStructorsInitializeVTables".
This method does not need to be passed a CodeGenFunction&, but it should take a complete CodeGenFunction::VPtr, not just this one random field from it.
As mentioned elsewhere, you can skip this entire loop if doStructorsInitializeVTables returns false.
Please call doStructorsInitializeVTables for RD above the call to getVTablePointers and then just skip the rest of the function if it returns false.
I won't argue with prioritizing Itanium over MS work, if that's your motivation, because that's obviously your call to make, and certainly that's how I would weigh things if I were doing this work. If you're interested in both, though, I would guess that vbptr assumptions would be particularly valuable — constructing objects and immediately converting them to a base class is pretty common in a lot of idioms.
yep, I thought that there will be problem with placing VPtr class, but I haven't noticed that CGCXXAABI includes CodeGenFunction.
CodeGenFunction is required for ItaniumABI
hmm, I think it is not true. This code checks if each base that we are initilizing/generating vptr assumption don't have novtable specifier. If one base has it, it doesn't mean that we don't have to do it for other base
No, it only checks whether VTableClass has novtable. VTableClass is the derived class for which we're initializing subobject v-tables, and it's invariant during getVTablePointers; that's why it's passed separately from BaseSubobject.
And if you think about how novtable works as a language feature, you'll see that it has to be that way. The purpose of "novtable" is to avoid emitting v-tables just for the short-term purposes of construction and destruction. It's not supposed to apply when initializing derived classes: eventually, the complete object does need all the v-tables to be initialized, or else you'll never be able to call virtual methods on it, meaning that it might as well not have any.
Just a couple tweaks and then LGTM.
Please use this comment:
// Generate vtable assumptions if we're constructing a complete object // with a vtable. We don't do this for base subobjects for two reasons: // first, it's incorrect for classes with virtual bases, and second, we're // about to overwrite the vptrs anyway.
Please also skip the call to getVTablePointers when doStructorsInitializeVPtrs, thanks.