Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV][RFC/WIP] Enable load clustering in SelectionDAG scheduler #74600

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

asb
Copy link
Contributor

@asb asb commented Dec 6, 2023

Builds on top of #73789.

Many changes are just noise (slightly different register choice), but there do seem to be some cases where additional clustering is taking place that I want to investigate further.

If we can get by with clustering in just the MachineScheduler, that would be preferable.

Posting at this point primarily for others to reference, not ready for direct review.

Copy link

github-actions bot commented Dec 6, 2023

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

@asb asb changed the title [RISCV][RFC/WIP] Enable store clustering in SelectionDAG scheduler [RISCV][RFC/WIP] Enable load clustering in SelectionDAG scheduler Dec 6, 2023
…ustering

Reordering based on the sort order of the MemOpInfo array was disabled
in <https://reviews.llvm.org/D72706>. However, it's not clear this is
desirable for al targets. It also makes it more difficult to compare the
incremental benefit of enabling load clustering in the selectiondag
scheduler as well was the machinescheduler, as the sdag scheduler does
seem to allow this reordering.

This patch adds a parameter that can control the behaviour on a
per-target basis.
Split out from llvm#73789. Clusters if the operations are within a cache
line of each other (as AMDGPU does in shouldScheduleLoadsNear). X86 does
something similar, but does `((Offset2 - Offset1) / 8 > 64)`. I'm not
sure if that's intentionally set to 512 bytes or if the division is in
error.

Adopts the suggestion from @wangpc-pp to query the cache line size and
use it if available.
@asb asb force-pushed the 2023q4-riscv-cluster-memops-3-with-sdag-clustering branch from d3850e7 to 044d1d8 Compare December 13, 2023 14:42
@asb asb force-pushed the 2023q4-riscv-cluster-memops-3-with-sdag-clustering branch from 044d1d8 to 91299d4 Compare December 13, 2023 14:42
@asb
Copy link
Contributor Author

asb commented Dec 13, 2023

Rebased on top of #73789 to make this easier to analyse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant