Page MenuHomePhabricator

Generating vptr assume loads
ClosedPublic

Authored by Prazek on Aug 7 2015, 6:51 PM.

Details

Summary

Generating vptr assumption loads for better devirtualization.

More here: http://lists.llvm.org/pipermail/cfe-dev/2015-July/044227.html

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Prazek updated this revision to Diff 31571.Aug 7 2015, 6:51 PM
majnemer edited edge metadata.Aug 10 2015, 12:13 PM

Some first round review comments.

lib/CodeGen/CGClass.cpp
1856–1857

Sentences should start with a capital letter and end with a period.

1863

Variables should be capitalized.

1867

The return should be on it's own line, please clang-format this.
Also, under which conditions is this case possible?

2125–2126

This kind of initialization is not very common in LLVM.

lib/CodeGen/CodeGenFunction.h
1312

Capital letter and period missing here.

1326–1333

Would it make more sense for these to live in CGClass ?

lib/CodeGen/ItaniumCXXABI.cpp
193–194

I'm not sure what this function is supposed to compute. Needed for what?

1402

Check what?

lib/CodeGen/MicrosoftCXXABI.cpp
223–228

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

Prazek added inline comments.Aug 10 2015, 12:19 PM
lib/CodeGen/CGClass.cpp
1867

Unfortunatelly clang-format puts short instructions in the same line

2125–2126

is It worth adding constructor just to change { with ( ?

lib/CodeGen/MicrosoftCXXABI.cpp
223–228

I don't know, I just took previous code assuming it is correct.

rsmith added inline comments.Aug 10 2015, 12:20 PM
include/clang/AST/VTableBuilder.h
387–391 ↗(On Diff #31571)

Please commit this cleanup separately.

lib/CodeGen/CGCXXABI.h
349–357

Please add documentation comments for these.

352

Typo Initilize -> Initialize

362–363

Comment is out of date.

lib/CodeGen/CGClass.cpp
1856–1857

Please start comments with a capital letter and end them with a full stop.

lib/CodeGen/ItaniumCXXABI.cpp
1402

Is this done? :)

rsmith added inline comments.Aug 10 2015, 12:22 PM
lib/CodeGen/CGClass.cpp
2125–2126

No, just add an = before the {.

djasper added inline comments.
lib/CodeGen/CGClass.cpp
1867

Not, it doesn't. At least not if you properly configure LLVM style.

Prazek marked 9 inline comments as done.Aug 10 2015, 7:38 PM
Prazek added inline comments.Aug 10 2015, 7:45 PM
lib/CodeGen/CGClass.cpp
1867

So mb I am doing something wrong, but I am using git-clang-format and get things like this

Prazek added inline comments.Aug 10 2015, 8:10 PM
lib/CodeGen/CodeGenFunction.h
1326–1333

What do You mean? These functions are defined in CGClass.cpp

lib/CodeGen/MicrosoftCXXABI.cpp
223–228

ok, I see. I just took code that suposed to work with microsoft and itanium abi. You are right

Prazek updated this revision to Diff 31766.Aug 10 2015, 9:22 PM
Prazek edited edge metadata.
Prazek updated this revision to Diff 31768.Aug 10 2015, 9:35 PM
nwilson added inline comments.
lib/CodeGen/CGClass.cpp
1887

Looks like another formatting/tab issue here.

2179

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:
./clang-format -style=llvm -lines="N:M" <FileToFormat> > tmp && cp tmp <FileToFormat>

(Just don't include the less than and greater around the FileToFormat.)

djasper added inline comments.Aug 10 2015, 9:53 PM
lib/CodeGen/CGClass.cpp
2179

You shouldn't need to do this by "hand". There is clang-format-diff.py / git-clang-format for that.

nwilson added inline comments.Aug 10 2015, 9:59 PM
lib/CodeGen/CGClass.cpp
2179

Good to know. Thanks!

Prazek marked 4 inline comments as done.Aug 11 2015, 11:54 AM
Prazek added inline comments.
lib/CodeGen/ItaniumCXXABI.cpp
193–194

isVirtualOffsetNeededForVtableField good enough?

Prazek marked 3 inline comments as done.Aug 11 2015, 11:58 AM
Prazek updated this revision to Diff 31846.Aug 11 2015, 12:01 PM
Prazek marked 4 inline comments as done.Aug 11 2015, 12:41 PM
rsmith added inline comments.Aug 13 2015, 11:10 AM
lib/CodeGen/CGCXXABI.h
349–357

Please also add documentation comments for the other two. =)

hfinkel added inline comments.
lib/CodeGen/CGClass.cpp
1857

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.
test/CodeGenCXX/vtable-assume-load.cpp
33

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

39

Don't match the TBAA metadata (leaving off everything after the comma before !tbaa should be fine).

Prazek added inline comments.Aug 13 2015, 5:46 PM
lib/CodeGen/CGClass.cpp
1857

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

rsmith added inline comments.Aug 13 2015, 6:07 PM
lib/CodeGen/CGClass.cpp
1857

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.

Prazek marked 5 inline comments as done.Aug 13 2015, 6:21 PM
Prazek marked an inline comment as done.Aug 13 2015, 6:43 PM
Prazek added inline comments.
lib/CodeGen/CGClass.cpp
1857

Well, it would be correct, but it would be useless. Just after calling base constructor, assume loads would appear, and after there would be new store to vptr.

rsmith added inline comments.Aug 13 2015, 6:50 PM
lib/CodeGen/CGClass.cpp
1857

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

Prazek marked 3 inline comments as done.Aug 13 2015, 7:01 PM
Prazek added inline comments.
lib/CodeGen/CGClass.cpp
1857

ok, now I get it

Prazek updated this revision to Diff 32122.Aug 13 2015, 7:05 PM
Prazek marked an inline comment as done.
rjmccall edited edge metadata.Aug 15 2015, 12:11 AM

Mostly LGTM. Are you going to emit assumptions for vbptrs in a separate patch?

lib/CodeGen/CGCXXABI.h
349–357

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

352

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.

lib/CodeGen/CGClass.cpp
1887

As mentioned elsewhere, you can skip this entire loop if doStructorsInitializeVTables returns false.

2179

Please call doStructorsInitializeVTables for RD above the call to getVTablePointers and then just skip the rest of the function if it returns false.

majnemer added inline comments.Aug 15 2015, 12:31 AM
test/CodeGenCXX/vtable-assume-load.cpp
41

This check line looks misspelled.

Mostly LGTM. Are you going to emit assumptions for vbptrs in a separate patch?

I wasn't planning to. I am focusing now on upgrading GVN for using new invariant.barrier metadata.

Mostly LGTM. Are you going to emit assumptions for vbptrs in a separate patch?

I wasn't planning to. I am focusing now on upgrading GVN for using new invariant.barrier metadata.

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.

Prazek marked an inline comment as done.Aug 16 2015, 5:27 PM
Prazek added inline comments.
lib/CodeGen/CGCXXABI.h
352

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
return NeedsVTTParameter(CGF.CurGD);

lib/CodeGen/CGClass.cpp
1887

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

Prazek marked an inline comment as done.Aug 16 2015, 5:55 PM

Mostly LGTM. Are you going to emit assumptions for vbptrs in a separate patch?

I wasn't planning to. I am focusing now on upgrading GVN for using new invariant.barrier metadata.

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.

It just wasn't in our plans. Main target was to devirtualize function calls much better, and I think that finishing the work from proposed paper will be much more valuable.

Prazek updated this revision to Diff 32262.Aug 16 2015, 7:27 PM
Prazek edited edge metadata.
Prazek updated this revision to Diff 32263.Aug 16 2015, 7:32 PM
rjmccall added inline comments.Aug 17 2015, 1:20 PM
lib/CodeGen/CGClass.cpp
1887

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.

Prazek marked 3 inline comments as done.Aug 17 2015, 2:03 PM
Prazek added inline comments.
lib/CodeGen/CGClass.cpp
1887

You are right. I changed things like You said - I only named it doStructorsInitilizeVPtrs

Prazek updated this revision to Diff 32343.Aug 17 2015, 2:07 PM
Prazek marked an inline comment as done.
Prazek marked 3 inline comments as done.

Just a couple tweaks and then LGTM.

lib/CodeGen/CGClass.cpp
1858

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

Please also skip the call to getVTablePointers when doStructorsInitializeVPtrs, thanks.

lib/CodeGen/ItaniumCXXABI.cpp
196

Typo: "Initialize".

Prazek updated this revision to Diff 32355.Aug 17 2015, 4:25 PM
Prazek marked 2 inline comments as done.
Prazek updated this revision to Diff 32363.Aug 17 2015, 7:16 PM

Fix for the PR24479

rsmith accepted this revision.Aug 17 2015, 7:24 PM
rsmith edited edge metadata.

LGTM

test/CodeGenCXX/vtable-assume-load.cpp
24

While you're here, maybe also remove the %"struct.test1::A"* (and likewise for the tests below).

This revision is now accepted and ready to land.Aug 17 2015, 7:24 PM
Prazek closed this revision.Aug 17 2015, 8:57 PM
Prazek marked 2 inline comments as done.Oct 27 2016, 4:01 AM