[PATCH] D94308: [MachineSink] SinkIntoLoop: analyse stores and aliases in between

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

`hasStoreBetween` is a little time consuming, so it is better to do some compiling time testing also.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:448
+  HasStoreCache.clear();
+  StoreInstrCache.clear();
+
----------------
Will it cause something wrong if we don't clear the cache here? 
 
I guess the cached data here should not impact the following sinking from the loop preheader to the loop body. 
Till now, all the sinkings are from the same depth loop or from a bigger depth loop to a smaller depth loop.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1226
+  // between that are aliased.
+  bool DontMoveAcrossStore = hasStoreBetween(I.getParent(), SinkTo, I);
+  LLVM_DEBUG(dbgs() << "LoopSink: Found store in between: "
----------------
I guess we only need to call `hasStoreBetween` for `load` instructions?


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1234
+
+  // 3) Check all instruction in the loop to see if there are aliases.
+  for (auto *BB : L->blocks()) {
----------------
This is a little strange, if we check there are no alias instructions in the whole loop to `I`, do we still need the above `hasStoreBetween` check? `I` is from the loop preheader, `SinkTo` is a block in the loop, `hasStoreBetween` is also checking alias for load/store instructions.


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

https://reviews.llvm.org/D94308