Page MenuHomePhabricator

ADT: Skip SmallVector::isReferenceToStorage when TakesParamByValue, NFC
ClosedPublic

Authored by dexonsmith on Jan 15 2021, 9:25 AM.

Details

Summary

Rename SmallVectorTemplateCommon::reserveForAndGetAddress() to
reserveForParamAndGetAddress(), clarifying that it should only be called
for "parameter values". Use that semantic information to skip calling
SmallVector::isReferenceToStorage() when TakesParamByValue (which is
constexpr).

This has no functional change, but likely speeds up operations like
SmallVector::push_back() for small POD-like types. Even if the
optimizer was smart enough to eliminate the call, this speeds up
compile-time since the call is now trivially dead.

Diff Detail

Event Timeline

dexonsmith created this revision.Jan 15 2021, 9:25 AM
dexonsmith requested review of this revision.Jan 15 2021, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 9:25 AM

Actually, I'm going to commit this immediately and not wait for review. Having caught up on my email that I missed yesterday, there's an active compile-time regression that this should fix. See https://reviews.llvm.org/D93779 for details.

(Happy to respond to comments post-commit of course.)

dexonsmith added a subscriber: nikic.

Actually, I'm going to commit this immediately and not wait for review. Having caught up on my email that I missed yesterday, there's an active compile-time regression that this should fix. See https://reviews.llvm.org/D93779 for details.

(Happy to respond to comments post-commit of course.)

Actually, no rush after all; the commits causing the compile-time regression were reverted in 33be50daa9ce1074c3b423a4ab27c70c0722113a.

@nikic, would you please check that this fixes the regression with the other three patches reapplied? (i.e., with 33be50daa9ce1074c3b423a4ab27c70c0722113a reverted?)

Assuming that looks good, I'll recommit the other three with this change.

dexonsmith added a subscriber: xbolva00.

@xbolva00, adding you here as well, since you reported the regression in https://reviews.llvm.org/D93779.

nikic accepted this revision.Jan 15 2021, 1:47 PM

I tested the original three patches plus this one, and the results look good (or at least in line with expectations): https://llvm-compile-time-tracker.com/compare.php?from=33be50daa9ce1074c3b423a4ab27c70c0722113a&to=972a64b1331d1b3022154297593837234a958d0b&stat=instructions

This revision is now accepted and ready to land.Jan 15 2021, 1:47 PM

I tested the original three patches plus this one, and the results look good (or at least in line with expectations): https://llvm-compile-time-tracker.com/compare.php?from=33be50daa9ce1074c3b423a4ab27c70c0722113a&to=972a64b1331d1b3022154297593837234a958d0b&stat=instructions

Thanks very much, I really appreciate it. I'll commit the revert with this added in.

This revision was landed with ongoing or failed builds.Jan 15 2021, 2:30 PM
This revision was automatically updated to reflect the committed changes.