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

[Proton][Dialect] Middle-end Proton operator definitions #5754

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

fywkevin
Copy link
Contributor

This PR is a follow-up of #5677 for proton compiler mid-end support, focusing on op definition. Specifically, we 1. added the tablegen definitions of the proton mid-end operators, 2. removed attributes of the front-end RecordOp (make it a true marker), 3. cleaned up the dialect's macro.

@fywkevin fywkevin requested review from Jokeren and CRobeck January 29, 2025 23:30
@fywkevin fywkevin requested a review from ptillet as a code owner January 29, 2025 23:30
Copy link
Contributor

@Jokeren Jokeren left a comment

Choose a reason for hiding this comment

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

Just one comment, should be easy to address

}

def PT_FinalizeOp : PT_Op<"finalize", [
MemoryEffects<[MemRead<SharedMemory>]>,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit confused to me why FinalizeOp has explicit resources but CircularRecordOp has the default resources.
TTG_MemDescType can refer to two different memory, either the old shared memory or the new tensor memory on Blackwell. I think this part has to be better clarified.

BTW, I agree it's also not clear in TritonOps.td. So maybe you can just go use GlobalMemory and SharedMemory considering that we only target GPUs for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit confused to me why FinalizeOp has explicit resources but CircularRecordOp has the default resources.

The rationale behind it is that "DefaultResource" is everything. FinalizeOp touches shared memory with read and global memory with read + write. Then, if another OP has shared memory read, you are good to re-order them but you can't reorder an OP that reads global memory (let's say the address is symbolic). For RecordOp , we are not supposed to move around it because it is for profiling. So I give it DefaultResource. For InitOp and FinalizeOp, compiler can do whatever it wants as long as it obeys SharedMemory/GlobalMemory side effects.

TTG_MemDescType can refer to two different memory, either the old shared memory or the new tensor memory on Blackwell. I think this part has to be better clarified.

I could explicitly describe that in the OP def's comment and has the OP verifier to check the mem_desc's address space, making sure it is shared mem. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's keep it as what it is and change it later

}

def PT_FinalizeOp : PT_Op<"finalize", [
MemoryEffects<[MemRead<SharedMemory>]>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's keep it as what it is and change it later

@Jokeren Jokeren merged commit 953d63c into triton-lang:proton-dev Jan 30, 2025
7 checks passed
fywkevin added a commit to fywkevin/triton that referenced this pull request Jan 31, 2025
…#5754)

This PR is a follow-up of triton-lang#5677 for proton compiler mid-end support,
focusing on op definition. Specifically, we 1. added the tablegen
definitions of the proton mid-end operators, 2. removed attributes of
the front-end `RecordOp` (make it a true marker), 3. cleaned up the
dialect's macro.

---------

Co-authored-by: Yuanwei Fang <[email protected]>
Jokeren pushed a commit that referenced this pull request Feb 1, 2025
This PR is a follow-up of #5677 for proton compiler mid-end support,
focusing on op definition. Specifically, we 1. added the tablegen
definitions of the proton mid-end operators, 2. removed attributes of
the front-end `RecordOp` (make it a true marker), 3. cleaned up the
dialect's macro.

---------

Co-authored-by: Yuanwei Fang <[email protected]>
Jokeren pushed a commit that referenced this pull request Feb 1, 2025
This PR is a follow-up of #5677 for proton compiler mid-end support,
focusing on op definition. Specifically, we 1. added the tablegen
definitions of the proton mid-end operators, 2. removed attributes of
the front-end `RecordOp` (make it a true marker), 3. cleaned up the
dialect's macro.

---------

Co-authored-by: Yuanwei Fang <[email protected]>
Jokeren pushed a commit that referenced this pull request Feb 5, 2025
This PR is a follow-up of #5677 for proton compiler mid-end support,
focusing on op definition. Specifically, we 1. added the tablegen
definitions of the proton mid-end operators, 2. removed attributes of
the front-end `RecordOp` (make it a true marker), 3. cleaned up the
dialect's macro.

---------

Co-authored-by: Yuanwei Fang <[email protected]>
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