-
Notifications
You must be signed in to change notification settings - Fork 77
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
Printer: add support to print types in MLIR style #136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great idea, but I would like to see some test cases. Afterward, we can merge 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this patch, I just added two nits comments.
I think I made a bad choice by having types closely linked with the values. While I think this makes more sense that the MLIR way, I believe that the change is not good enough to differ from MLIR.
I think this option should be the one by default, though we should give time to people to update their project with this version, before changing everything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a test or two, otherwise LGTM.
Actually, my bad, I want to add more things to it before merging it. I'm not sure how to make this a draft PR though. |
You can make it a draft in the |
Okay, that should now be ready for review, I made quite some changes since the last review! I added a unit test file, and a simple test file for LLVM. The LLVM test is quite small, mostly because I did not wanted to implement the printing for each dialect, we can do this step by step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!A short test of printing and reparsing with MLIR also worked fine.
I think you did not yet handle UnitAttrs, which I feel should be part of this PR as well.
57cb8d0
to
732bf1f
Compare
I added differential testing in mlir-conversion/ops.xdsl ! |
Does this now pass all the mlir related tests or is there still something missing? Unfortunately, I do currently not have an up to date python bindings build and can thus not test it locally. |
It works on my machine, so LGTM! |
We might consider to add an LLVM build to the CI and pin the LLVM version with a submodule. Running these test in the CI might be beneficial. |
This brings the xDSL output closer to the syntax MLIR is using.