[PATCH] D79391: [DSE] Remove noop stores in MSSA.

Sun May 24 13:52:02 PDT 2020

fhahn added a comment.

In D79391#2052317 <https://reviews.llvm.org/D79391#2052317>, @zoecarver wrote:

> Execution time:
>
>   Tests: 198
>   Short Running: 97 (filtered out)
>   Remaining: 101
>   Metric: exec_time
>  
>   Program                                        stats_opt stats_master diff
>  
>   test-suite...ks/Prolangs-C++/life/life.test     2.30      3.01       30.9%
>   test-suite...arks/mafft/pairlocalalign.test    12.66     15.31       20.9%
>   test-suite.../Benchmarks/Ptrdist/bc/bc.test     0.68      0.83       20.7%
>   test-suite...Source/Benchmarks/sim/sim.test     2.81      3.38       20.2%
>   test-suite...urce/Applications/aha/aha.test     1.25      1.02       -18.4%
>   test-suite...s/Ptrdist/anagram/anagram.test     0.62      0.72       16.9%
>   test-suite.../Benchmarks/nbench/nbench.test     1.32      1.55       16.9%
>   test-suite...ks/VersaBench/8b10b/8b10b.test     4.31      5.02       16.4%
>   test-suite...nchmarks/llubenchmark/llu.test     3.19      3.68       15.1%
>   test-suite.../Applications/lemon/lemon.test     0.87      0.75       -14.1%
>   test-suite...tions/lambda-0.1.3/lambda.test     3.50      3.04       -13.1%
>   test-suite.../VersaBench/ecbdes/ecbdes.test     1.81      2.02       11.7%
>   test-suite...netbench-url/netbench-url.test     2.97      3.31       11.3%
>   test-suite...hmarks/VersaBench/bmm/bmm.test     2.34      2.60       11.3%
>   test-suite...marks/SciMark2-C/scimark2.test    28.48     31.68       11.2%
>   Geomean difference                                                    nan%
>          stats_opt  stats_master       diff
>   count  99.000000  101.000000    99.000000
>   mean   3.921419   4.036584      0.035598
>   std    4.557934   4.988885      0.071592
>   min    0.617100   0.635000     -0.184291
>   25%    1.575900   1.549400     -0.003149
>   50%    3.090300   3.072300      0.022620
>   75%    4.272200   4.313500      0.070729
>   max    28.946700  31.948900     0.308558
>
>
> Compile time:
>
>   Tests: 198
>   Metric: compile_time
>  
>   Program                                        stats_opt stats_master diff
>  
>   test-suite...rks/tramp3d-v4/tramp3d-v4.test    43.40     52.93       22.0%
>   test-suite...arks/mafft/pairlocalalign.test    26.05     30.58       17.4%
>   test-suite...ce/Benchmarks/PAQ8p/paq8p.test     4.32      4.97       15.1%
>   test-suite...frame_layout/frame_layout.test     2.47      2.81       13.5%
>   test-suite...marks/7zip/7zip-benchmark.test   122.67    131.64        7.3%
>   test-suite...fice-ispell/office-ispell.test     3.99      4.28        7.2%
>   test-suite...earch/office-stringsearch.test     0.44      0.47        6.8%
>   test-suite...netbench-crc/netbench-crc.test     0.20      0.21        6.8%
>   test-suite...nsumer-lame/consumer-lame.test    11.69     12.47        6.6%
>   test-suite...math/automotive-basicmath.test     0.34      0.32       -6.3%
>   test-suite...lowfish/security-blowfish.test     0.64      0.68        6.2%
>   test-suite...comm-adpcm/telecomm-adpcm.test     0.15      0.16        6.2%
>   test-suite.../Prolangs-C++/ocean/ocean.test     0.38      0.35       -6.0%
>   test-suite.../Trimaran/enc-pc1/enc-pc1.test     0.19      0.20        5.4%
>   test-suite...DOE-ProxyApps-C/CoMD/CoMD.test     2.84      2.69       -5.3%
>   Geomean difference                                                    0.4%
>           stats_opt  stats_master        diff
>   count  198.000000  198.000000    198.000000
>   mean   5.349209    5.432042      0.004814
>   std    13.699984   14.112571     0.038243
>   min    0.053100    0.052200     -0.063188
>   25%    0.312425    0.314125     -0.025223
>   50%    1.604600    1.594650      0.001472
>   75%    3.347775    3.419850      0.026751
>   max    122.668900  131.644500    0.219606
>
>
> Question: it would seem like the compile-time actually got a lot better (which is odd). Am I reading the data incorrectly? Shouldn't smaller numbers be better?

Runtimes/execution times depend a lot on the system and can be quite noisy. For comparable numbers you have to make sure that the system noise is reduced to a minimum (and you don't build & run the benchmarks in parallel).

I would suggest to focus on the statistics about removed stores and how the patch impacts them. With respect to compile-time, I don't think there's much to worry about with the patch in its current form..

Also, I recently found out that EarlyCSE also does some simple form of load-store forwarding (https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/EarlyCSE.cpp#L1214). It might be worth consolidating the DSE logic in the DeadStoreElimination pass in the future, but that's a discussion for the future.

================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1846
+  // access may point to it. So, pretend like we've already checked it.
+  ToCheck.insert(KillingDef);
+  ToCheck.insert(Current);
----------------
IIUC the MemoryPHI handling should be optional, right? I would recommend splitting it off in a separate patch, as smaller patches make it generally easier to review/submit.

================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1897
+                          << '\n');
+        break;
+      }
----------------
shouldn't that be `continue`? (Test case would be a scenario where we first remove a no-op store and then some other stores by another def in State.MemDefs.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391