Page MenuHomePhabricator

[ThinLTO] Add a recursive step in Metadata lazy-loading
ClosedPublic

Authored by mehdi_amini on Jan 15 2017, 4:17 PM.

Details

Summary

Without this, we're stressing the RAUW of unique nodes,
which is a costly operation. This is intended to limit
the number of RAUW, and is very effective on the total
link-time of opt with ThinLTO, before:

real 4m4.587s  user 15m3.401s  sys 0m23.616s

after:

real 3m25.261s user 12m22.132s sys 0m24.152s

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini retitled this revision from to [ThinLTO] Add a recursive step in Metadata lazy-loading.
mehdi_amini updated this object.
mehdi_amini added reviewers: tejohnson, pcc.
mehdi_amini added a subscriber: llvm-commits.

Add a comment, and handles correctly uniquing cycles (even if I didn't encounter any when bootstrapping clang)

Note that I gave the timing for the *full* link, if I focus on the time spent in "materializeMetadata()" is goes from 215s to 37s, almost a 6x improvement.

tejohnson accepted this revision.Jan 17 2017, 6:52 AM

Nice improvement - is this related to the regression you saw in your measurements on the effectiveness of lazy loading metadata? A few comments/suggestions below.

Add a comment, and handles correctly uniquing cycles (even if I didn't encounter any when bootstrapping clang)

Do we have a test case that exposes this issue? If not, it would be good to add

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
836 ↗(On Diff #84508)

The comment is a little confusing, since we are in fact creating a temporary just above. Maybe change to something like "instead of keeping the temporary which requires a later RAUW".

This revision is now accepted and ready to land.Jan 17 2017, 6:52 AM
This revision was automatically updated to reflect the committed changes.

Nice improvement - is this related to the regression you saw in your measurements on the effectiveness of lazy loading metadata? A few comments/suggestions below.

Yes, it recovers this and goes beyond.

Do we have a test case that exposes this issue? If not, it would be good to add

Added! Thanks it was a good idea as it shown that the patch was wrong (I was creating a temp for the operand about to be lazy-loaded instead of the parent).