[PATCH] D93694: [MachineLICM] SinkIntoLoop: analyse successive store instructions

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Yeah, so thanks again for looking at this. After looking more into this, I don't think this change at the moment makes much sense.

Sinking instructions is a bit of a mess as we have LoopSink, MachineSink, and MachineLICM all doing this (in different ways, and/or different use cases). This change here and function `HasSuccessiveStoreInst()` that I added is very similar to `hasStoreBetween()` in MachineSink. It's actually better, so I would like reuse that. As different strategies I have considered hoisting that out to a new MachineLoopUtils helper, but there's state (caching) that doesn't make this very convenient. I think the better alternative is to move function `SinkIntoLoop()` from MachineLICM to MachineSink because that is actually a more natural place for it, and there seems to be more/better infrastructure in MachineSink to do this (like `hasStoreBetween()` that I mentioned earlier).

A first prep step for this D94082 <https://reviews.llvm.org/D94082>, which creates a new helper isLoopInvariant() which we can then also use in MachineSink.  After this, I would like to repurpose this ticket and move `SinkIntoLoop()` from MachineLICM to MachineSink if we agree that this makes sense. That should be a NFC, and then as a third step I would like to make the change that I wanted to make.


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

https://reviews.llvm.org/D93694