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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi,

Looks reasonable but I'd like to have an idea of the compile time impact of this patch.

Make sure to fix the Lint comments as well.

Cheers,
-Quentin



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:348
     ToSplit.clear();
+    StoreInstrCache.clear();
+    HasStoreCache.clear();
----------------
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.


================
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) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86864