Page MenuHomePhabricator

Emit available_externally vtables opportunistically

Authored by Prazek on May 23 2017, 5:04 AM.

Diff Detail


Event Timeline

Prazek created this revision.May 23 2017, 5:04 AM
Prazek updated this revision to Diff 99897.May 23 2017, 6:30 AM

Removed debug print

rjmccall added inline comments.May 26 2017, 7:52 AM
160 ↗(On Diff #99897)

Please use an exhaustive switch. You can put llvm_unreachable in the other cases.

905 ↗(On Diff #99897)

I would add a CGM.shouldOpportunisticallyEmitVTables() that can just check the optimization level, and then you can use that to avoid even collecting v-tables like this if you don't need to.

1377 ↗(On Diff #99897)

It's worth adding a header comment to this explaining that it runs after EmitDeferred() and therefore is not allowed to create new references to things that need to be emitted lazily.

Prazek updated this revision to Diff 100530.May 27 2017, 3:45 AM
Prazek marked 3 inline comments as done.
  • Addressing John's comments

Thanks for the comments :)

160 ↗(On Diff #99897)

Should I implement this for RTTI fields? Or maybe leave a fixme comment that it could also work for some other fields in vtable, but is not currently used?

rjmccall added inline comments.May 28 2017, 3:56 PM
160 ↗(On Diff #99897)

I think your precondition of isUsedFunctionPointerKind() is fine, you don't need to handle RTTI in this function.

This does raise the interesting question, though, of whether this approach is safe for lazily-emitted RTTI. I guess it currently works because IRGen doesn't use the normal deferred-emission mechanism for RTTI objects, so if the RTTI object is lazily-emitted, we'll just eagerly emit it instead of potentially ending up with an illegal reference to external RTTI. You should add a comment to the appropriate place in CGRTTI (i.e. where we fill in the global definition) saying that this optimization may need adjustment if we ever switch to using deferred emission for some reason.

169 ↗(On Diff #100530)

By "exhaustive" I mean that you should list out all the cases instead of using default. It means that someone who adds a new kind of v-table entry will get alerted to fix this switch. There's only five other cases, it's not too bad.

Prazek marked an inline comment as done.May 29 2017, 10:19 AM
Prazek added inline comments.
169 ↗(On Diff #100530)

Oh right, that make sense

Prazek marked 4 inline comments as done.May 29 2017, 10:51 AM
Prazek added inline comments.
160 ↗(On Diff #99897)

I couldn't find CGRTTI, so I added comment in Itanium and Microsoft ABI where we create RTTI fields, but maybe the CodeGenModule::GetAddrOfRTTIDescriptor would be a better place instead?

I also added small note to the EmitVTablesOpportunistically about this.

Prazek updated this revision to Diff 100623.May 29 2017, 10:51 AM
Prazek marked an inline comment as done.
  • Final changes
dblaikie added inline comments.
1383–1385 ↗(On Diff #100623)

Perhaps this:

assert(OpportunisticVTables.empty() || shouldOpportunisticallyEmitVTables() && ... )

(it's a bit odd to have a condition that only goes to an assert - rather than having both conditions inside the assertion)

Prazek updated this revision to Diff 100641.May 29 2017, 2:28 PM

changed assert

Prazek marked an inline comment as done.May 29 2017, 2:28 PM
rjmccall accepted this revision.May 31 2017, 5:58 PM

Looks great, thanks!

This revision is now accepted and ready to land.May 31 2017, 5:58 PM
Prazek marked an inline comment as done.Jun 1 2017, 1:02 AM
This revision was automatically updated to reflect the committed changes.