[PATCH] D59514: [PGO] Profile guided code size optimization.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added inline comments.


================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:62
+    "pgso", cl::Hidden, cl::init(true),
+    cl::desc("Enable the profile guided size optimization. "));
+
----------------
eraman wrote:
> Perhaps better to default to false, do robust performance testing and flip the option. You've mentioned code size reduction in one binary in the description, but good to validate there are no  performance regressions in spec.
Here's SPEC data.

400.perlbench
base: 512.81 ±0.564 (8 trials, 7.8 robust values)
test: 512.16 ±0.420 (8 trials, 7.1 robust values)
diff: -0.13% ±0.124%
Possible significant effect (p=0.047)

401.bzip2
base: 532.43 ±1.145 (8 trials, 7.9 robust values)
test: 532.81 ±0.962 (8 trials, 8.0 robust values)
diff: +0.07% ±0.254%
Not significant (p=0.567)

403.gcc
base: 313.08 ±1.356 (8 trials, 7.8 robust values)
test: 312.63 ±1.183 (8 trials, 7.5 robust values)
diff: -0.14% ±0.519%
Not significant (p=0.558)

429.mcf
base: 239.39 ±3.018 (8 trials, 8.0 robust values)
test: 239.41 ±2.067 (8 trials, 8.0 robust values)
diff: +0.01% ±1.386%
High variance between trials

445.gobmk
base: 560.88 ±0.613 (8 trials, 8.0 robust values)
test: 560.46 ±0.143 (7 trials, 6.0 robust values)
diff: -0.07% ±0.103%
Not significant (p=0.142)

456.hmmer
base: 602.40 ±0.140 (8 trials, 7.7 robust values)
test: 602.45 ±0.074 (8 trials, 7.5 robust values)
diff: +0.01% ±0.024%
Not significant (p=0.441)

458.sjeng
base: 607.02 ±0.957 (8 trials, 7.5 robust values)
test: 606.24 ±0.599 (8 trials, 7.5 robust values)
diff: -0.13% ±0.167%
Not significant (p=0.123)

462.libquantum
base: 291.97 ±4.636 (8 trials, 8.0 robust values)
test: 290.80 ±3.281 (8 trials, 7.6 robust values)
diff: -0.40% ±1.765%
High variance between trials

464.h264ref
base: 795.35 ±0.658 (8 trials, 7.9 robust values)
test: 795.05 ±0.345 (8 trials, 8.0 robust values)
diff: -0.04% ±0.085%
Not significant (p=0.354)

471.omnetpp
base: 274.20 ±5.967 (8 trials, 6.1 robust values)
test: 271.89 ±7.348 (8 trials, 6.6 robust values)
diff: -0.85% ±3.043%
High variance between trials

473.astar
base: 426.80 ±0.802 (8 trials, 7.8 robust values)
test: 425.75 ±0.526 (8 trials, 6.1 robust values)
diff: -0.25% ±0.202%
Possible significant effect (p=0.021)

483.xalancbmk
base: 250.64 ±1.038 (8 trials, 8.0 robust values)
test: 250.19 ±0.530 (8 trials, 6.9 robust values)
diff: -0.18% ±0.422%
Not significant (p=0.380)





================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:538
+          L->getHeader()->getParent()->optForSize() ||
+          (PSI && BFI && PSI->shouldOptimizeForSize(L->getHeader()->getParent(),
+                                                    L->getHeader(), BFI));
----------------
davidxl wrote:
> is BFI null check needed?
It is now.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:206
+      L->getHeader()->getParent()->optForSize() ||
+      (PSI && BFI &&
+       PSI->shouldOptimizeForSize(L->getHeader()->getParent(),
----------------
davidxl wrote:
> Do you need to check BFI here? It is already checked in shouldOptimizeForSize
It is now.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59514/new/

https://reviews.llvm.org/D59514