[PATCH] D120330: [MachineSink] Fix CFG walk in clobber check (PR53990)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D120330#3339787 <https://reviews.llvm.org/D120330#3339787>, @shchenz wrote:

> In D120330#3339665 <https://reviews.llvm.org/D120330#3339665>, @nikic wrote:
>
>> In D120330#3339261 <https://reviews.llvm.org/D120330#3339261>, @shchenz wrote:
>>
>>> Can we check why the instruction is sunk from a shallower block `From` to a deeper block `To`?  `MachineSinking::isProfitableToSinkTo()` should not allow this?
>>
>> This happens because the loop is irreducible. The loop depth check is based on MachineLoopInfo, which only handles natural loops. So sinking into irreducible cycles is not prevented by the profitability check.
>
> Is it possible to exclude this irreducible loop case in `isProfitableToSinkTo` and add some assertions/checks in `hasStoreBetween`?

It should be possible to do this by switching MachineSink from using MachineLoopInfo to MachineCycleInfo, which supports irreducible cycles. I think this allows a better profitability decision, but I'm not entirely sure that it would be a sufficient correctness condition, as irreducible cycles are DFS-order dependent. In any case, MachineCycleInfo is a new addition that is not actually used anywhere yet, so I don't think this would be appropriate for an LLVM 14 backport.

> The posted patch may largely increase the compile time as now more blocks are checked. Compile-time issue happened before in https://reviews.llvm.org/D86864#2469289

This is still subject to the existing limits on the walks. If there are compile-time issues, then those limits are too large (which, looking at them now, may well be the case: the limits allow walking up to 20 * 2000 instructions, which does not seem like a sensible limit.)


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

https://reviews.llvm.org/D120330