[PATCH] D71717: [MachineScheduler] Ignore artificial edges when forming store chains

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D71717#1846557 <https://reviews.llvm.org/D71717#1846557>, @foad wrote:

> In D71717#1845450 <https://reviews.llvm.org/D71717#1845450>, @rampitec wrote:
>
> > I have realized that BaseMemOpClusterMutation::apply() in fact does not check all control dependencies and just breaks at the first one. I.e. this change just skips some SDeps preferring another one. More or less we are lucky to find a correct SDep which may form a useful chain. We may be not that lucky if order of the SDeps is different and we would somehow use another register (for example data register). We probably need a callback to check if that register belongs to pointer operand and skip it otherwise. Alternatively we may need a full search to find a best SDep in the list.
> >
> > This is LGTM, but can you please add cluster_load_valu_cluster_store function from the testcase in D73509 <https://reviews.llvm.org/D73509>? At the moment stores are not properly clustered:
> >
> >   flat_store_dword v[2:3], v4
> >   v_add_u32_e32 v1, 1, v5
> >   flat_store_dword v[2:3], v6 offset:16
> >   flat_store_dword v[2:3], v1 offset:8
> >   flat_store_dword v[2:3], v0 offset:24
> >   
>
>
> I can add it but I get different results. With D73509 <https://reviews.llvm.org/D73509>:
>
>   	flat_store_dword v[2:3], v4
>   	flat_store_dword v[2:3], v6 offset:16
>   	flat_store_dword v[2:3], v0 offset:8
>   	flat_store_dword v[2:3], v7 offset:24
>
>
> With this patch:
>
>   	flat_store_dword v[2:3], v4
>   	flat_store_dword v[2:3], v0 offset:8
>   	flat_store_dword v[2:3], v6 offset:16
>   	flat_store_dword v[2:3], v7 offset:24
>
>
> I can see from the debug output that all four stores are being clustered now.
>
> Do you prefer tests that just check the generated code, instead of checking the -debug-only output? It seems to me that there is a high chance of stores getting clustered by accident, even if the scheduler is not doing the right thing. E.g. the scheduler could do nothing at all and the test would still pass, because the loads and stores are already in the correct order before scheduling!


Right, the sort is different. But now it is clustered and before it was not. I think it is OK to use debug output in this case. I agree it is very easy to get them accidentally clustered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71717