[PATCH] D86864: [MachineSinking] sink more profitable loads

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks very much for your review. @qcolombet  Update accordingly. I collected some compiling time numbers for llvm test-suite. No obivious degradation found.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:348
     ToSplit.clear();
+    StoreInstrCache.clear();
+    HasStoreCache.clear();
----------------
qcolombet wrote:
> Would it make sense to preserve the cache as long as we don't move stores, calls and loads with ordered memory ref?
> 
> I'm worried about the compile time impact of this patch so I am wondering if we want to be smarter about invalidating the cache.
Good idea. But seems we still need to clear the cache before we return from `MachineSinking::runOnMachineFunction`. (`MachineSink` instance is shared with different callings to `MachineSinking::runOnMachineFunction` for different functions), otherwise I meet some weird memory issue.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:919
+    // To post dominates BB, it must be a path from block From.
+    if (PDT->dominates(To, BB)) {
+      for (MachineInstr &I : *BB) {
----------------
qcolombet wrote:
> While we traverse BB, should we create an entry for the pair (To, BB).
> 
> Put differently, how is the compile time impact of this patch and do we have to do more to avoid computations?
I collected some data from PowerPC, seems the compiling time difference is not obivious in llvm test-suite. the tool hides some small diff tests. Among them, I saw some tests takes 10s and some of them takes more than 30s, but no big reg for them. The biggest diff 9.6% and 7.4%, compile time are around 0.1s. And these two degradations can not reproduciable in other runs.

```
./utils/compare.py -m compile_time base.json fix.json  
Tests: 309
Metric: compile_time

Program                                        base  fix   diff 
                                                                  
 test-suite...enchmarks/Stanford/Queens.test       9.6%
 test-suite...d-warshall/floyd-warshall.test       7.4%
 test-suite...ImageProcessing/Blur/blur.test      -4.7%
 test-suite...SubsetBRawLoops/lcalsBRaw.test       2.7%
 test-suite...math/automotive-basicmath.test     -2.5%
 test-suite.../Benchmarks/Stanford/Perm.test      -2.4%
 test-suite...rks/tramp3d-v4/tramp3d-v4.test     -2.2%
 test-suite...ow-dbl/GlobalDataFlow-dbl.test      -2.2%
 test-suite...enchmarks/Stanford/RealMM.test      -2.0%
 test-suite...ctions-flt/Reductions-flt.test      -1.8%
 test-suite...t/StatementReordering-flt.test      -1.7%
 test-suite.../Trimaran/enc-rc4/enc-rc4.test      -1.7%
 test-suite...l/StatementReordering-dbl.test      1.6%
 test-suite...pansion-dbl/Expansion-dbl.test      -1.4%
 test-suite...bl/IndirectAddressing-dbl.test      -1.2%
 Geomean difference                                           nan%
            base        fix        diff
count  309.000000  309.000000  277.000000
mean   2.696964    2.685603   -0.003232  
std    6.283106    6.251353    0.009456  
min    0.000000    0.000000   -0.047321  
25%    0.077944    0.077862   -0.005599  
50%    0.291912    0.289780   -0.003404  
75%    2.475318    2.478657   -0.001450  
max    64.257411   64.088470   0.095601  
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86864