This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Look through aliases in cache key calculations
ClosedPublic

Authored by george.burgess.iv on Nov 29 2018, 9:39 AM.

Details

Summary

We can sometimes get incomplete cache keys (e.g. from not accounting for used type IDs/etc. by an aliasee) when using aliases. This patch aims to fix that.

If you want me to build my own specific test for this, I'm happy to do so. Tweaking this existing test seems to work just as well, though. Please let me know :)

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson added inline comments.Nov 30 2018, 10:53 AM
lib/LTO/LTO.cpp
189 ↗(On Diff #175887)

By skipping the alias here, we don't end up hashing its own live bit. I have a suggestion below.

223 ↗(On Diff #175887)

This loop will call AddUsedThings for every global defined in this module, so presumably this already handles both the alias and the base object.

230 ↗(On Diff #175887)

Presumably this is where we were missing the base object, because we may have the alias in the import list but not its base object (but we will actually import the alias as a copy of the base object). My suggestion would be to check if the summary is an alias here, and if so call AddUsedThings a second time on the base object summary.

george.burgess.iv marked 4 inline comments as done.

Addressed feedback

Thanks for the feedback!

lib/LTO/LTO.cpp
230 ↗(On Diff #175887)

SGTM.

The _or_null in my new patch was out of caution rather than actual test failures. Please let me know if I should be able to assume S != nullptr, and I'm happy to drop that. :)

tejohnson accepted this revision.Dec 3 2018, 11:34 AM

LGTM

lib/LTO/LTO.cpp
230 ↗(On Diff #175887)

I would have thought we should get a non-null S, but the existing code handles it (the early return in AddUsedThings), so it is ok with me to keep the _or_null here.

This revision is now accepted and ready to land.Dec 3 2018, 11:34 AM
This revision was automatically updated to reflect the committed changes.