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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Thanks for looking!

In D94308#2538202 <https://reviews.llvm.org/D94308#2538202>, @shchenz wrote:

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

This is really where I think/hope the caching of the result is going to help, so that essentially the analysis is done once, and the rest are lookups. And since we run the loop sinking as a second second, I hope it's almost free.
But I said "hope" a few times, and it's a good suggestion to check this, which I will do! Also, you made a good point about clearing this cache, see inline.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:448
+  HasStoreCache.clear();
+  StoreInstrCache.clear();
+
----------------
shchenz wrote:
> 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.
> 
Good point. I haven't seen it doing anything wrong, I didn't try very hard though, but I put it there because I saw the preheader block being modified in the first sinking step. Loop sinking as a second step, then tries to sink instructions from that modified preheader block, so I thought about just clearing it just to be sure. But now thinking about it more, I don't think this can cause problems, so after verifying this I will remove this clearing of the cache.


================
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: "
----------------
shchenz wrote:
> I guess we only need to call `hasStoreBetween` for `load` instructions?
Yep, will change that.


================
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()) {
----------------
shchenz wrote:
> 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.
Agreed that this has become messy. I.e., it's doing more work than necessary. 
We need to analyse all instructions in preheader block, which is what we do in 1).
Then, we need to analyse all blocks/instructions in the loop, which is what we do in 3).
I think we can now indeed get rid of 2), which would be useful for more generic cases but here in our case the preheader is directly dominating the loop blocks so don't need to analyse a path in between that.


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

https://reviews.llvm.org/D94308