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] Add Initial Frontend and Target Backend Infrastructure For Proton Dialect #5506

Merged
merged 30 commits into from
Jan 6, 2025

Conversation

CRobeck
Copy link
Contributor

@CRobeck CRobeck commented Dec 28, 2024

Implement initial basic infrastructure for the Proton Dialect added in #5119

This PR extends the initial boilerplate MLIR Dialect code to the Triton frontend and target backends - currently just lowered to a no-op.

@Jokeren Jokeren self-assigned this Dec 28, 2024
@CRobeck CRobeck changed the title [Proton][Dialect] Add Initial Frontend Support For Proton Dialect [Proton][Dialect] Add Initial Frontend and Target Backend Support For Proton Dialect Dec 28, 2024
@CRobeck CRobeck marked this pull request as ready for review December 28, 2024 15:04
@CRobeck
Copy link
Contributor Author

CRobeck commented Dec 28, 2024

cc: @fywkevin

lib/Conversion/TritonGPUToLLVM/CMakeLists.txt Outdated Show resolved Hide resolved
lib/Conversion/TritonToTritonGPU/CMakeLists.txt Outdated Show resolved Hide resolved
void populateProtonPatterns(TritonGPUTypeConverter &typeConverter,
RewritePatternSet &patterns) {
MLIRContext *context = patterns.getContext();
patterns.add<GenericOpPattern<triton::proton::RecordOp>>(typeConverter,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be taken out if we don't consider the frontend changes for now

Copy link
Contributor

@Jokeren Jokeren Dec 30, 2024

Choose a reason for hiding this comment

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

Maybe we don't need any conversion here anyway because proton's inputs are scalars but not tensors. Could you please double check? @CRobeck

Copy link
Contributor Author

@CRobeck CRobeck Dec 30, 2024

Choose a reason for hiding this comment

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

They are scalars but could envision a case where we pass in tensors in the future - if we want to pass in a Triton tensor object for example for object specific tracing. I don't think the conversion hurts anything here but acknowledge it could potentially be misleading.

You're right though that I can remove this entire function (populateProtonPatterns) and the test still passes.

python/src/ir.cc Outdated
@@ -1603,6 +1606,10 @@ void init_triton_ir(py::module &&m) {
llvm::StringRef(prefix));
self.create<PrintOp>(prefixAttr, hex, values, isSigned);
})
.def("create_proton_record",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take the python api changes out of this PR. It's not determined yet and probably not very important in the short term. Users, like @pawelszczerbuk, can modify the GPU IR to instrument record ops

Copy link
Contributor Author

@CRobeck CRobeck Dec 28, 2024

Choose a reason for hiding this comment

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

This is mostly just to have a template in place for the future Proton front end development to interact with the upper level Triton Python kernel code - we can always modify the interface later but how the Proton Python frontend fit together with the Triton IR builder wasn't super intuitive at first so having this here I think is helpful for the moment even if we ultimately change the API in the future. It also gives us some more testing avenues. 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.

The concern I have is that we don't have any real functionalities associated with these record ops anyway. Usually, we add a frontend op when there's at least some functionalities implemented. For example, the most recent tl.gather op. Though its initial commit doesn't address all problems and concerns, frontend users are able to use the op in their kernels but not just placeholders. So IMHO it makes more sense to add the op, and mark it as experimental, after the lowering has been implemented.

Copy link
Contributor Author

@CRobeck CRobeck Dec 29, 2024

Choose a reason for hiding this comment

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

What about putting things in a dev module with some warnings about them being under development and not intentioned for use outside the development team:

https://github.com/triton-lang/triton/pull/5506/files#diff-054c0ddf64263bf99d77a237a61deeb79c0cd3e4289a03eba948ce833f1cdce0R17

https://github.com/triton-lang/triton/pull/5506/files#diff-bc216e7a68b5a4545f1ff69bfbf5d0fa64adb6b1acaca6ea0f366f47e93942ebR25

Then that gives us an easy method to develop/test in a way that turning them "on" is just a matter of moving them from the dev module to the experimental namespace?

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 much better now!

Let's just move it under proton.language without using the dev class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think create_proton_record can be moved to triton_proton.cc

Copy link
Contributor Author

@CRobeck CRobeck Dec 30, 2024

Choose a reason for hiding this comment

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

Updated to be under proton.language.

Also I think create_proton_record can be moved to triton_proton.cc

I think this would cause a lot of code disruption. TritonOpBuilder is a unique ptr wrapper around the MLIR OpBuilder. So I think no matter what we'd have to find way to pass that object to triton_proton.cc either through moving the definition of TritonOpBuilder out of ir.cc into a header file or possibly through a helper function defined in ir.cc and called from triton_proton.cc which seems like an equal or more amount of complexity to the existing code.

Maybe we can just invoke the generic MLIR OpBuilder from triton_proton.cc? But then I think we'd be mixing and matching builder objects in the code generator?

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it now. Let's move it to the last line of all ops and leave a comment:

// proton ops
.def("create_proton_record",
...

It's a problem for all third party backends that they cannot define custom ops. We will have to think about a better solution.

Copy link
Contributor Author

@CRobeck CRobeck Dec 31, 2024

Choose a reason for hiding this comment

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

Updated.

It's a problem for all third party backends that they cannot define custom ops. We will have to think about a better solution.

Right, there has to be an upper level Triton Op that the backends can specialize but I could not find a way to have a wholly custom third party backend Op. Given that TritonOpBuilder is defined in ir.cc the only solution I see is to pull that class/object out into a standalone module that can be passed to each backend so that they all operate on the same MLIR builder instance.

@CRobeck CRobeck changed the title [Proton][Dialect] Add Initial Frontend and Target Backend Support For Proton Dialect [Proton][Dialect] Add Initial Frontend and Target Backend infrastructure For Proton Dialect Dec 30, 2024
@CRobeck CRobeck changed the title [Proton][Dialect] Add Initial Frontend and Target Backend infrastructure For Proton Dialect [Proton][Dialect] Add Initial Frontend and Target Backend Infrastructure For Proton Dialect Dec 30, 2024
python/src/ir.cc Outdated
@@ -1603,6 +1606,10 @@ void init_triton_ir(py::module &&m) {
llvm::StringRef(prefix));
self.create<PrintOp>(prefixAttr, hex, values, isSigned);
})
.def("create_proton_record",
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 much better now!

Let's just move it under proton.language without using the dev class.

python/src/ir.cc Outdated
@@ -1603,6 +1606,10 @@ void init_triton_ir(py::module &&m) {
llvm::StringRef(prefix));
self.create<PrintOp>(prefixAttr, hex, values, isSigned);
})
.def("create_proton_record",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think create_proton_record can be moved to triton_proton.cc

python/src/ir.cc Show resolved Hide resolved
python/src/ir.cc Show resolved Hide resolved
void populateProtonPatterns(TritonGPUTypeConverter &typeConverter,
RewritePatternSet &patterns) {
MLIRContext *context = patterns.getContext();
patterns.add<GenericOpPattern<triton::proton::RecordOp>>(typeConverter,
Copy link
Contributor

@Jokeren Jokeren Dec 30, 2024

Choose a reason for hiding this comment

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

Maybe we don't need any conversion here anyway because proton's inputs are scalars but not tensors. Could you please double check? @CRobeck

third_party/proton/proton/profile.py Outdated Show resolved Hide resolved
third_party/proton/proton/language.py Outdated Show resolved Hide resolved
python/src/ir.cc Outdated
@@ -1603,6 +1606,10 @@ void init_triton_ir(py::module &&m) {
llvm::StringRef(prefix));
self.create<PrintOp>(prefixAttr, hex, values, isSigned);
})
.def("create_proton_record",
Copy link
Contributor

Choose a reason for hiding this comment

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

I get it now. Let's move it to the last line of all ops and leave a comment:

// proton ops
.def("create_proton_record",
...

It's a problem for all third party backends that they cannot define custom ops. We will have to think about a better solution.

third_party/proton/proton/__init__.py Outdated Show resolved Hide resolved
third_party/proton/proton/profile.py Outdated Show resolved Hide resolved
@Jokeren
Copy link
Contributor

Jokeren commented Dec 31, 2024

It looks good to me now mostly. Do you want to postpone merging into main a bit until Yuanwei has taken a look?

@CRobeck
Copy link
Contributor Author

CRobeck commented Dec 31, 2024

Yes, that's fine. I don't think should be anything too controversial.

@zhanglx13 zhanglx13 merged commit dcad5ac into triton-lang:main Jan 6, 2025
7 checks passed
@CRobeck CRobeck deleted the proton_record branch January 8, 2025 02:37
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.

3 participants