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

TTIR constant materialization #1548

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Conversation

azecevicTT
Copy link
Contributor

ConstantOp in the TTIR dialect has a ConstantLike trait so constants can be materialized into ttir.constant operation. We already have one use case, where GetDimensionSizeOp is folded into Attribute, so instead of decomposition, a folding into Attribute and materialization into ConstantOp takes a more natural approach.

For all future use cases of FooOpToConstantOp conversions, instead of implementing pattern rewriting, it's enough to fold FooOp into mlir::ElementsAttr.

Copy link
Contributor

@mrakitaTT mrakitaTT left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adding this! I've planned to do this long time ago but wasn't sure if that will do the trick without other changes to enable canonicalization. I also wasn't sure if we can rely that folders will always be activated during compilation and remove explicit conversions, mlir documentation wasn't very helpful on this, did you find a confirmation somewhere?

@azecevicTT
Copy link
Contributor Author

Nice, thanks for adding this! I've planned to do this long time ago but wasn't sure if that will do the trick without other changes to enable canonicalization. I also wasn't sure if we can rely that folders will always be activated during compilation and remove explicit conversions, mlir documentation wasn't very helpful on this, did you find a confirmation somewhere?

@mrakitaTT Yeah, I had to dive into MLIR's source code, during legalization it first tries with fold, and only then try with patterns as a fallback.
https://mlir.llvm.org/doxygen/DialectConversion_8cpp_source.html : 2033.

Copy link
Contributor

@sdjordjevicTT sdjordjevicTT left a comment

Choose a reason for hiding this comment

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

Great change!

Copy link
Contributor

@nsmithtt nsmithtt left a comment

Choose a reason for hiding this comment

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

This looks great! Perhaps we can add a test that exercises this case?

@azecevicTT
Copy link
Contributor Author

@nsmithtt There are already some tests that cover get_dimension_size_op conversions, though none of them cover ttir.get_dimension_size -> ttir.constant directly, they all go ttir.get_dimension_size -> ttnn.full where ttir.constant is the intermediate step. Thanks for pointing that out, I will add it soon.

@azecevicTT azecevicTT force-pushed the azecevic/constant-materializer branch from 0d2758b to 4c38555 Compare December 11, 2024 14:29
@azecevicTT azecevicTT force-pushed the azecevic/constant-materializer branch 4 times, most recently from 2ec80f3 to 7222b62 Compare December 12, 2024 14:49
@azecevicTT azecevicTT force-pushed the azecevic/constant-materializer branch from 7222b62 to 9ab0add Compare December 12, 2024 16:10
@azecevicTT azecevicTT merged commit ed84a56 into main Dec 12, 2024
21 checks passed
azecevicTT added a commit that referenced this pull request Dec 17, 2024
* TTIR constant materialization

* GetDimensionSizeOp removed from decomp and realized through fold + materialization

* Added test for get_dimension_size -> constant conversion
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.

4 participants