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

Remove MatmulParams::rotate_ldmatrix_out_of_main_loop #3337

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

jacobhinkle
Copy link
Collaborator

I can't find any commit in which this option was ever actually used. This is the commit where the option was originally introduced: https://github.com/csarofeen/pytorch/pull/2488/files#diff-e7a5a84a2cfeddeb15669f07105bdb3722a796600ea9e1f2eb25afb29283457eR22 We've gone this long without the ability to disable loop rotation, so either we should change the condition in the schedulers to respect it, or just remove it.

I can't find any commit in which this option was ever actually used.
This is the commit where the option was originally introduced:
https://github.com/csarofeen/pytorch/pull/2488/files#diff-e7a5a84a2cfeddeb15669f07105bdb3722a796600ea9e1f2eb25afb29283457eR22
We've gone this long without the ability to disable loop rotation, so
either we should change the condition in the schedulers to respect it,
or just remove it.
@jacobhinkle
Copy link
Collaborator Author

!test

Copy link
Collaborator

@rdspring1 rdspring1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, we always rotate the main loop if circular buffering is enabled. Or is that separate from ldmatrix?

https://github.com/NVIDIA/Fuser/blob/main/csrc/scheduler/ampere_multi_matmul.cpp#L1330-L1342


The code changes look fine to me.

@jacobhinkle
Copy link
Collaborator Author

IIUC, we always rotate the main loop if circular buffering is enabled. Or is that separate from ldmatrix?

Yeah, I think the idea was that we'd rotate the main loop when circular buffering as you said, but probably for extra flexibility the option was introduced. As an alternative to this PR, we could just as easily start respecting this option and add the condition to the heuristic. That would let us measure the effect of loop rotation.

@rdspring1
Copy link
Collaborator

What is the benefit of loop rotation?

I'm not sure if ldmatrix is high priority for hopper, so it is mostly an ampere feature.
I wouldn't mind having fewer flags, so removing it is fine by me.

@jacobhinkle jacobhinkle merged commit 6b3ee4f into main Nov 5, 2024
47 checks passed
@jacobhinkle jacobhinkle deleted the remove_rotate_ldmatrix_option branch November 5, 2024 17:05
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.

2 participants