[PATCH] D123995: [MachineSink] replace MachineLoop with MachineCycle

Mon May 30 21:10:31 PDT 2022

shchenz added a comment.

In D123995#3545464 <https://reviews.llvm.org/D123995#3545464>, @nikic wrote:

> In D123995#3461553 <https://reviews.llvm.org/D123995#3461553>, @shchenz wrote:
>
>>> The change description says that MachineSink is "sensitive" to cycle depth. Is it only the performance of the resulting program, or does it affect correctness too?
>>
>> I think the depth check should only impact the performance in machinesink. However there are some codes(`hasStoreBetween`) which does not work right for irreducible loops, with this patch, we can get correct behavior. But I think we need more investigation for that function. I plan to do more investigation later to avoid errors.
>
> Just to double check, did you verify that hasStoreBetween() works fine with the new implementation? I'm slightly concerned that we may be reintroducing the miscompile depending on how CycleInfo gets chosen.

Yes, I did some check before, with the machinecycle, we should be ok now for `hasStoreBetween()` (I am not 100% sure though). 
1: We will not sink instructions from BBFrom in a cycle leading by HeaderA to a BBTo in another cycle leading by HeaderB, as `BBFrom` must dominate `BBTo`, but HeaderA and HeaderB does not need to be dominated. 
2: We will also not sink instruction from predecessor block of header cycle to a block inside the cycle no matter the cycle is reducible or irreducible.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995