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

Fri May 29 03:13:31 PDT 2020

fhahn added a comment.

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

> > Might be interesting to see what happens with MSSA disabled for EarlyCSE.
>
> I passed `CXXFLAGS="-mllvm -enable-dse-memoryssa=true -mllvm -earlycse-mssa-optimization-cap=0"` to cmake and re-built and ran the tests. It doesn't look like that had any impact, though. I still get `Unknown metric 'dse.NumNoopStores'` which I assume means none were recorded.

Hm I think cmake might not pick up CXXFLAGS, if it's not the initial run. Probably better to pass `-DCMAKE_C_FLAGS=/-DCMAKE_CXX_FLAGS=`. Or just flip the default for `EnableMemorySSA ` (which I usually do :P) and rebuild clang/libLTO.

To verify, you can check `dse.NumFastStores`. There should be a lot of changes with/without MemorySSA. For example, with `-O3 -flto`:

  ~/projects/test-suites/test-suite/utils/compare.py --filter-hash -m dse.NumFastStores  base.json mssa.json
  Tests: 237
  Same hash: 104 (filtered out)
  Remaining: 133
  Metric: dse.NumFastStores

  Program                                        base   mssa   diff
   test-suite...-typeset/consumer-typeset.test   162.00 891.00 450.0%
   test-suite...INT2000/164.gzip/164.gzip.test    15.00  67.00 346.7%
   test-suite...ks/Prolangs-C/agrep/agrep.test     2.00   8.00 300.0%
   test-suite...000/183.equake/183.equake.test    10.00  40.00 300.0%
   test-suite...lications/sqlite3/sqlite3.test    22.00  76.00 245.5%
   test-suite...CFP2006/433.milc/433.milc.test    64.00 195.00 204.7%
   test-suite...000/197.parser/197.parser.test     3.00   9.00 200.0%
   test-suite...chmarks/Olden/power/power.test    11.00  33.00 200.0%
   test-suite...lications/ClamAV/clamscan.test    72.00 210.00 191.7%
   test-suite.../Applications/SPASS/SPASS.test    48.00 139.00 189.6%
   test-suite...T2006/445.gobmk/445.gobmk.test    19.00  52.00 173.7%
   test-suite...Source/Benchmarks/sim/sim.test     2.00   5.00 150.0%
   test-suite...abench/jpeg/jpeg-6a/cjpeg.test     9.00  22.00 144.4%
   test-suite...ks/McCat/12-IOtest/iotest.test     6.00  14.00 133.3%
   test-suite...nsumer-jpeg/consumer-jpeg.test    12.00  27.00 125.0%
   Geomean difference                                           nan%

For noop stores, it looks like there are the following nook-store changes for the configuration above:

  test-suite...:: External/Povray/povray.test    NaN   1.00  nan%
  test-suite...CFP2000/177.mesa/177.mesa.test    NaN   1.00  nan%
  test-suite...000/183.equake/183.equake.test    NaN  11.00  nan%
  test-suite...CFP2006/433.milc/433.milc.test    NaN   1.00  nan%
  test-suite...006/447.dealII/447.dealII.test    NaN   2.00  nan%
  test-suite...006/453.povray/453.povray.test    NaN  13.00  nan%
  test-suite...6/482.sphinx3/482.sphinx3.test    NaN   1.00  nan%
  test-suite.../CINT2000/176.gcc/176.gcc.test    NaN   6.00  nan%
  test-suite...0/253.perlbmk/253.perlbmk.test    NaN  21.00  nan%
  test-suite...0.perlbench/400.perlbench.test    NaN  24.00  nan%
  test-suite...T2006/445.gobmk/445.gobmk.test    NaN  25.00  nan%
  test-suite...lications/ClamAV/clamscan.test    NaN   2.00  nan%
  test-suite.../Applications/SPASS/SPASS.test    NaN   2.00  nan%
  test-suite...pplications/oggenc/oggenc.test    NaN   2.00  nan%
  test-suite...s/MallocBench/cfrac/cfrac.test    NaN   6.00  nan%
  test-suite...chmarks/MallocBench/gs/gs.test    NaN   1.00  nan%
  test-suite...TimberWolfMC/timberwolfmc.test    NaN   1.00  nan%
  test-suite.../Prolangs-C/loader/loader.test    NaN   1.00  nan%

As mentioned earlier, I think this looks good, once the comments for the tests are addressed :)

================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/simple.ll:34
 ;
-  %g_value = load i32, i32* %g_addr, align 4
   store i32 -1, i32* @g, align 4
----------------
zoecarver wrote:
> fhahn wrote:
> > Is there a reason for this change in the source (and some of the ones below)? I think we should keep the existing tests as is and add additional cases as required.
> The load and store here would be removed. I wasn't sure if the noop store was inadvertent and wanted to keep the test as similar as possible to what I thought it was testing (that neither store is removed). If you'd rather I can remove the changes and update the `CHECK` comments instead. 
I don't think the current version of the patch would remove anything for this test case (and likely the other modified ones). `%g_addr` and `@g` may alias I think, so the defining access of `store i32 %g_value, i32* %g_addr, align 4` would be `store i32 -1, i32* @g, align 4` and the defining access of ` %g_value = load i32, i32* %g_addr, align 4` would be `entry`.

Most existing tests intentionally cover cases to exercise various code paths/guard against problematic scenarios encountered in the past. Therefor it would be good to preserve the original input and update the check lines, if there are any changes (those can then be checked during the review). 

I am not sure if we could remove anything here, as it would require proving it is safe to remove them in all possible alias scenarios for `%g_addr` and `@g`. If they don't alias, we can remove the store to `g_addr`. If they alias exactly (= complete overwrite), we could remove `store i32 %g_value, i32* %g_addr, align 4`, but only if we also remove `store i32 -1, i32* @g, align 4`. But we cannot remove that store if they don't alias.

================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/simple.ll:627
+; Remove redundant store if loaded value is in another block.
+define i32 @test26(i1 %c, i32* %p) {
+; CHECK-LABEL: @test26(
----------------
Might be good to move the new tests for coop-stores into a separate test file (and please remove the tests from simple-todo.ll)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391