This is an archive of the discontinued LLVM Phabricator instance.

Variable auto-init: split out small arrays
ClosedPublic

Authored by jfb on Mar 3 2019, 8:09 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.Mar 3 2019, 8:09 AM
jfb updated this revision to Diff 189085.Mar 3 2019, 8:11 AM
jfb marked an inline comment as done.
  • typo
jfb added a comment.Mar 3 2019, 8:11 AM

I'll do a few size diffs to double-check that this also pays off. @glider can you also check that it doesn't regress what you've been looking at?

test/CodeGenCXX/auto-var-init.cpp
1133 ↗(On Diff #189084)

This was a typo, inadvertently capturing %0.

glider added inline comments.Mar 4 2019, 7:32 AM
test/CodeGenCXX/auto-var-init.cpp
1025 ↗(On Diff #189085)

This check fails for me locally.

jfb updated this revision to Diff 189159.Mar 4 2019, 9:20 AM
  • Fix test labels.
jfb marked 2 inline comments as done.Mar 4 2019, 9:21 AM
jfb added inline comments.
test/CodeGenCXX/auto-var-init.cpp
1025 ↗(On Diff #189085)

Apologies, I played around with the labels and forgot to fix them before sending the patch.

glider added a comment.Mar 4 2019, 9:32 AM

The change itself looks good.
It doesn't seem to regress kernel performance on ARM64. I haven't got to testing on x86 yet, but don't anticipate any problems either.

lib/CodeGen/CGDecl.cpp
1206 ↗(On Diff #189159)

Is the second expression being moved to line 1206 a result of clang-format? Otherwise it'll migrate back at some point.

jfb marked 3 inline comments as done.Mar 4 2019, 9:39 AM
jfb added inline comments.
lib/CodeGen/CGDecl.cpp
1206 ↗(On Diff #189159)

Yes, the slightly longer name pushes it past 80 columns, and I just auto-format stuff before uploading.

I can do this change separately, I just noticed that the name I originally used was now misleading because vectors aren't scalars :)

glider accepted this revision.Mar 4 2019, 9:42 AM
glider added inline comments.
lib/CodeGen/CGDecl.cpp
1206 ↗(On Diff #189159)

Up to you, this doesn't matter IMO :)

This revision is now accepted and ready to land.Mar 4 2019, 9:42 AM
jfb marked an inline comment as done.Mar 4 2019, 12:16 PM

Comparing clang stage2 in release mode, with an without this change, we see a 408 byte size difference, which is ~nothing. Here's details, nothing surprising:

$ /s/bloaty/bloaty -d sections /s/llvm1/llvm/stage2/bin/clang-9  -- /s/llvm2/llvm/stage2/bin/clang-9
     VM SIZE                                 FILE SIZE
 --------------                           --------------
   +44% +1.84Ki [__TEXT]                  +1.84Ki   +45%
   +65%    +408 [__LINKEDIT]                    0  [ = ]
  -0.0%      -8 Table of Non-instructions      -8  -0.0%
  -0.0%     -48 Symbol Table                  -48  -0.0%
  -0.0%    -120 Export Info                  -120  -0.0%
  -0.0%    -232 String Table                 -232  -0.0%
  -0.0%    -832 __TEXT,__text                -832  -0.0%
  -0.0% -1.03Ki __TEXT,__cstring          -1.03Ki  -0.0%
  [ = ]       0 TOTAL                        -408  -0.0%

$ /s/bloaty/bloaty -d symbols /s/llvm1/llvm/stage2/bin/clang-9  -- /s/llvm2/llvm/stage2/bin/clang-9
     VM SIZE                                                                  FILE SIZE
 --------------                                                            --------------
   +45% +1.84Ki [__TEXT]                                                   +1.84Ki   +45%
  +0.0%    +280 [__LINKEDIT]                                                  -128  -0.0%
  +0.4%     +96 clang::ento::check::ASTDecl<>::_checkDecl<>()                  +96  +0.4%
  +2.9%     +48 emitStoresForConstant()                                        +48  +2.9%
  -0.9%     -16 clang::Sema::DeclareGlobalAllocationFunction()                 -16  -0.9%
  -0.0%     -16 clang::ento::check::PostStmt<>::_checkStmt<>()                 -16  -0.0%
  -8.3%     -78 clang::AnalyzerOptions::getCheckerStringOption()               -78  -8.3%
 -12.5%     -80 clang::ento::registerPaddingChecker()                          -80 -12.5%
  -6.3%     -96 clang::ASTContext::GetBuiltinType()                            -96  -6.3%
 -37.8%    -205 clang::AnalyzerOptions::getCheckerIntegerOption()             -205 -37.8%
 -49.5%    -269 clang::AnalyzerOptions::getCheckerBooleanOption()             -269 -49.5%
  -0.0%    -480 llvm::APInt::toString()::Digits                               -480  -0.0%
 -67.4%    -496 clang::driver::toolchains::NetBSD::addLibCxxIncludePaths()    -496 -67.4%
  -0.0%    -576 [__TEXT,__cstring]                                            -576  -0.0%
  [ = ]       0 TOTAL                                                         -408  -0.0%

For codebases with more small arrays this likely plays better, but at least we know it's not a regression.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 5:26 PM