Page MenuHomePhabricator

Avoid redundant work when computing vtable vcall visibility
ClosedPublic

Authored by tejohnson on Nov 17 2020, 8:05 PM.

Details

Summary

Add a Visited set to avoid repeatedly processing the same base classes
in complex class hierarchies. This cut down the compile time of one
source file from >12min to ~1min.

Diff Detail

Event Timeline

tejohnson created this revision.Nov 17 2020, 8:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2020, 8:05 PM
tejohnson requested review of this revision.Nov 17 2020, 8:05 PM

Adding another reviewer - @wmi can you take a look? This is a straightforward compile time fix.

wmi added inline comments.Nov 24 2020, 9:34 AM
clang/lib/CodeGen/CGVTables.cpp
1301–1302

If a CXXRecordDecl is visited twice, the visibility returned in the second visit could be larger than necessary. Will it change the final result? If it will, can we cache the visibility result got in the first visit instead of returning the max value?

tejohnson added inline comments.Nov 24 2020, 9:38 AM
clang/lib/CodeGen/CGVTables.cpp
1301–1302

The recursive callsites compute the std::min of the current TypeVis and the one returned by the recursive call. So returning the max guarantees that there is no effect on the current TypeVis. Let me know if the comment can be clarified (that's what I meant by "so that it has no effect on the min visibility computed below ...". Note that the initial non-recursive invocation always has an empty Visited set.

wmi accepted this revision.Nov 24 2020, 9:55 AM

LGTM.

clang/lib/CodeGen/CGVTables.cpp
1301–1302

I see. That makes sense! I didn't understand the location meant by "computed below by the recursive caller." Your explanation "initial non-recursive invocation always has an empty Visited set" helps a lot. It means the immediate result of GetVCallVisibilityLevel may change, but the result for the initial invocation of the recursive call won't be changed.

This revision is now accepted and ready to land.Nov 24 2020, 9:55 AM

Improve comment

tejohnson added inline comments.Nov 24 2020, 10:16 AM
clang/lib/CodeGen/CGVTables.cpp
1301–1302

I've tried to clarify the comment accordingly

wmi accepted this revision.Nov 24 2020, 10:45 AM
wmi added inline comments.
clang/lib/CodeGen/CGVTables.cpp
1301–1302

Thanks for making the comment more clear.