[PATCH] D93530: [DSE] Add support for not aligned begin/end

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Just realized that I missed this one.

With respect to the performance numbers you posted earlier, I am not sure if that is caused by the patch. For SPEC2000/SPEC2006 & MultiSource with -O3 -flto on X86, only 12 benchmarks have binary changes and they are all very minor. The total number of stores is as below (that's with `--filter-hash --all`).  What's a bit curious is that in 2 cases we are left with a tiny number of more stores, but the number of eliminated stores stays the same.

It seems like we do not have a stats counter to measure the number of times we successfully shortened operations.  The patch basically looks good to me, but it would be good to understand where the additional stores are coming from.

  Same hash: 225 (filtered out)
  Remaining: 12
  Metric: dse.NumRemainingStores
  
  Program                                        base     patch    diff
   test-suite...006/450.soplex/450.soplex.test   8700.00  8701.00   0.0%
   test-suite...006/447.dealII/447.dealII.test   90533.00 90535.00  0.0%
   test-suite.../CINT2000/176.gcc/176.gcc.test   18291.00 18291.00  0.0%
   test-suite.../CINT2006/403.gcc/403.gcc.test   36093.00 36093.00  0.0%
   test-suite...nal/skidmarks10/skidmarks.test   1161.00  1161.00   0.0%
   test-suite...lications/ClamAV/clamscan.test   10078.00 10078.00  0.0%
   test-suite...lications/viterbi/viterbi.test   102.00   102.00    0.0%
   test-suite...marks/7zip/7zip-benchmark.test   35503.00 35503.00  0.0%
   test-suite...oxyApps-C/RSBench/rsbench.test   145.00   145.00    0.0%
   test-suite...oxyApps-C/XSBench/XSBench.test   120.00   120.00    0.0%
   test-suite...nsumer-jpeg/consumer-jpeg.test   5972.00  5972.00   0.0%
   test-suite...nsumer-lame/consumer-lame.test   4501.00  4501.00   0.0%



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1096
+  if (IsOverwriteEnd) {
+    uint64_t Off =
+        offsetToAlignment(uint64_t(LaterStart - EarlierStart), PrefAlign);
----------------
IIUC this is the adjustment to keep `ToRemoveStart` aligned to `PrefAlign`, right? Might be worth a comment be worth a comment.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1132
+                    << *EarlierWrite << "\n  KILLER ( offset " << ToRemoveStart
+                    << ", " << ToRemoveSize << ")\n");
 
----------------
perhaps it would be worth adjusting the message to make it clear that the second number is the size we shorten by?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93530