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

Thu Feb 24 01:15:32 PST 2022

nikic added a comment.

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

>> 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.
>
> Yes, I agree changing MachineLoopInfo to MachineCycleInfo would be a little risky for backport. Could we just simply mark it as not profitable in `isProfitableToSinkTo` if `MBB` or `SuccToSinkTo` is in a loop that contains IrreducibleCFG.
>
> ShrinkWrap.cpp has an example check:
>
>   ReversePostOrderTraversal<MachineBasicBlock *> RPOT(&*MF.begin());
>   if (containsIrreducibleCFG<MachineBasicBlock *>(RPOT, *MLI)) {
>
> For this case, IMO, not allowing the sink should be the right choice as block To is in a deeper loop? Apart from the load/store sink, we may sink some other instructions to a hot loop. And not sure other queries from MachineLoopInfo in this pass have the right behavior for an irreducible loop.

This is possible, but without MachineCycleInfo, this would be a very crude check: We would completely disable sinking for any functions that contain irreducible control flow (I don't think containsIrreducibleCFG is legal to use on a sub-graph). This seems potentially problematic to me, because irreducible control-flow is more common in the machine back-end than the middle-end. Here's a quick test: https://gist.github.com/nikic/e45e179e9a84c18de86c041158d3067e

I'm generally okay with doing that though, excluding irreducible control-flow entirely is certainly the most conservative fix.

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

https://reviews.llvm.org/D120330