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

Add support for custom printing and parsing #115

Merged
merged 2 commits into from
May 13, 2022
Merged

Add support for custom printing and parsing #115

merged 2 commits into from
May 13, 2022

Conversation

math-fehr
Copy link
Collaborator

Should fix #113

This PR allows operations to override print and parse to essentially implement custom parsing. It also open a bit the API of the printer and parser class.
This PR does not add custom format for dialects defined in xDSL, so there should not be any breaking change.

There is an example of its usage in tests/printer_test.py

@math-fehr math-fehr requested review from Dinistro and webmiche May 11, 2022 21:08
Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

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

Looks very good for me 👍

What would be even cooler is if we could declaratively describe what the printed op should look and it would generate parser and printer for free. Like the PlusCostumFormatOp is specificied as:

"test.add" "lhs" + "rhs"

# | | | | |/ _` |/ _` | '_ \ / _ \/ __| __| |/ __|
# | |_| | | (_| | (_| | | | | (_) \__ \ |_| | (__
# |____/|_|\__,_|\__, |_| |_|\___/|___/\__|_|\___|
# |___/
Copy link
Collaborator

Choose a reason for hiding this comment

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

How long did you spend on those? xD

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope that he used a tool for that ;P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't worry, I use figlet ;)

@Dinistro
Copy link
Collaborator

At a first glance this seems to look good. Unfortunately, I don't have time today for an in depth review. Not sure if that would be required though.

@webmiche
Copy link
Collaborator

In terms of op definition generations, @nicolasvasilache pointed me today to OpDSL(Git), a DSL that allows defining operations from which linalg.generic is produced. Might be interesting to understand how this correlates with what we are trying to do :)

return PlusCustomFormatOp.create(operands=[lhs, rhs],
result_types=result_types)

def print(self, printer: Printer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Food for thought: It might be cool, if we could just write something like

printer.print(f"{self.lhs} + {self.rhs}")

here, tough I guess that does not work in our current infrastructure as the SSAValue names are not known...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the problem here is that we cannot just do a print(self.lhs), since we need the context.
I'll simplify it with printer.print(self.lhs, " + ", self.rhs) if you want!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that would look better I feel. You mentioned ssomething about refactoring printer and parser at some point, maybe we can make it such that this would be possible afterward?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, though I'm not completely sure that I'll be able to do that :/
The problem is that I don't know how we can print an SSA Value without a context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though what we could do is to write assembly_format = "$lhs + $rhs" if you want.
Though we could do this in a separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah let's leave this one as is, I think its fine like this.

@math-fehr
Copy link
Collaborator Author

In terms of op definition generations, @nicolasvasilache pointed me today to OpDSL(Git), a DSL that allows defining operations from which linalg.generic is produced. Might be interesting to understand how this correlates with what we are trying to do :)

Yes, OpDSL is kind of related to IRDL as well, though OpDSL is specialized for linalg.
Compared to what we are doing, this OpDSL would essentially be a front-end DSL that targets IRDL.

@math-fehr
Copy link
Collaborator Author

I simplified things a bit, now it looks like printer.print(" ", self.lhs, " + ", self.rhs).
Of course, we would like to have something like assembly_format = "$lhs + $rhs" at some point, but we should do it in a separate PR I feel, since this can be a lot of work.

@math-fehr
Copy link
Collaborator Author

If this is good for you, I think we can merge it!
This shouldn't have any breaking change (since I don't think we were using the private functions of the parser/printer).

@webmiche webmiche merged commit e9420ef into main May 13, 2022
@math-fehr math-fehr deleted the custom-format branch July 20, 2022 12:45
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.

Custom Parser and Printer
3 participants