xDSL: Make dialect registration more intuitive #231

webmiche · 2022-11-25T11:34:40Z

This PR drafts a new way of registering (and working with) dialects within xDSL. Firstly, it gives dialects its own abstraction (a Dialect class) that contains two lists, one for operations, one for attributes. Furthermore, it adds the call ctx.register_dialect to the MLContext class. Using this function, one can now register dialects by writing:

ctx.register_dialect(func)

instead of:

f = Func(ctx)

where f is basically just a useless variable.

webmiche · 2022-11-25T11:35:43Z

@math-fehr could you check the filecheck tests on this? I had weird behavior on my machine earlier. Will retry this afternoon after making sure my MLIR is on the proper version.

review-notebook-app · 2022-11-25T11:39:06Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

xdsl/xdsl_opt_main.py

xdsl/ir.py

georgebisbas

Looks good, only minor comments

xdsl/ir.py

superlopuh · 2022-11-26T21:31:42Z

If we merge this before applying to all the other uses of _ = [\w]\(ctx\) then I hope we can make the second PR soon after this one, so the code isn't inconsistent for too long. It's a welcome change.

webmiche · 2022-11-27T10:57:56Z

If we merge this before applying to all the other uses of _ = [\w]\(ctx\) then I hope we can make the second PR soon after this one, so the code isn't inconsistent for too long. It's a welcome change.

I am planning to wait for Mathieu's feedback on this and then rewrite the rest within this PR. Then, we should not run into problems.

math-fehr

Yes, we definitely want that, thanks!
I added a comment on the way I think we should structure the dialects.

xdsl/ir.py

math-fehr · 2022-11-28T06:29:57Z

@math-fehr could you check the filecheck tests on this? I had weird behavior on my machine earlier. Will retry this afternoon after making sure my MLIR is on the proper version.

I'll take a look tomorrow!

webmiche · 2022-11-28T09:26:49Z

@math-fehr could you check the filecheck tests on this? I had weird behavior on my machine earlier. Will retry this afternoon after making sure my MLIR is on the proper version.

I'll take a look tomorrow!

Actually, I looked into this a bit deeper and found the cause. I fixed it in #232. Basically, we had a test that was testing mlir custom prints, but we only print generic form, so that always failed. I guess it still makes sense if you run it on your machine, then we have at least run it on two machines making weird MLIR versioning issues less likely.

xdsl/dialects/func.py

xdsl/ir.py

webmiche · 2022-11-28T10:58:56Z

Yes, we definitely want that, thanks! I added a comment on the way I think we should structure the dialects.

I restructured with that in mind. Could you re-review, @math-fehr ?

math-fehr · 2022-11-28T14:13:32Z

Hmm, I feel that this is quite hacky to be honest!
Could we instead have a Dialect class, that has the _operations and _attributes fields?
Then we can define our dialect with func = Dialect([FuncOp, Call, Return], [])?
I feel this would make more sense than taking the _operations and _attributes global values.

webmiche · 2022-11-28T16:32:21Z

Yes, I agree. I think I missunderstood you suggestion. This design makes more sense.

Still, iterators won't work AFAIU, considering that you suggestion uses the global class.

math-fehr · 2022-11-28T16:54:53Z

Still, iterators won't work AFAIU, considering that you suggestion uses the global class.

Why wouldn't them?
We could add this in the Dialect class:

@property
def operations(self) -> Iterator[type[Operation]]:
    return iter(self._operations)

unless I misunderstood something?

webmiche · 2022-11-29T07:44:27Z

I actually coded this last night and realized that we could do exactly that 👍

Small question: Why did you annotated with type[Operation]? I don't think I quite get the difference between Operation and type[Operation] in type annotations.

codecov · 2022-11-29T07:44:41Z

Codecov Report

Base: 81.48% // Head: 81.35% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (596d73b) compared to base (d0644ab).
Patch coverage: 86.66% of modified lines in pull request are covered.

❗ Current head 596d73b differs from pull request most recent head cdc3e8d. Consider uploading reports for the commit cdc3e8d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   81.48%   81.35%   -0.14%     
==========================================
  Files          29       29              
  Lines        5413     5347      -66     
  Branches      912      910       -2     
==========================================
- Hits         4411     4350      -61     
+ Misses        762      757       -5     
  Partials      240      240

Impacted Files	Coverage Δ
tests/mlir_converter_test.py	`4.25% <0.00%> (ø)`
xdsl/ir.py	`70.73% <87.50%> (+0.51%)`	⬆️
tests/pattern_rewriter_test.py	`99.60% <100.00%> (ø)`
tests/printer_test.py	`97.99% <100.00%> (ø)`
tests/rewriter_test.py	`100.00% <100.00%> (ø)`
tests/use_test.py	`100.00% <100.00%> (ø)`
xdsl/dialects/arith.py	`75.35% <100.00%> (-2.05%)`	⬇️
xdsl/dialects/builtin.py	`80.19% <100.00%> (-1.25%)`	⬇️
xdsl/dialects/func.py	`88.37% <100.00%> (-1.43%)`	⬇️
xdsl/dialects/memref.py	`71.55% <100.00%> (+3.83%)`	⬆️
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

webmiche · 2022-11-29T13:09:31Z

I am guessing that the reason why the code coverage decreases with this PR is that I am removing tested lines from func, therefore decreasing the total number of lines, while keeping the number of uncovered lines the same.

(Sanity check) Does this make sense, @georgebisbas?

georgebisbas · 2022-11-29T13:53:04Z

Do not worry you are fine:

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   81.16%   81.14%   -0.02%     
==========================================
  Files          28       28              
  Lines        5367     5374       +7     
  Branches      912      914       +2     
==========================================
+ Hits         4356     4361       +5     
- Misses        771      772       +1     
- Partials      240      241       +1

It is normal and minor

webmiche · 2022-11-30T16:08:57Z

xdsl/ir.py

+    def get_attributes(self) -> Iterator[Attribute]:
+        return iter(self.__attributes)
+
+    def __call__(self, ctx: MLContext) -> None:


This function allows backward compatibility. Do we want that here?

I would say that we shouldn't keep it, especially since we do not have many users yet.
What do you think @georgebisbas ? You know more than us about these things.

Keeps compatibility for what exactly?

Previously, we registered dialects with _ = Arith(ctx). This still allows to register dialects like this, hence keeping backward compatibility.

ah ok, then keep it, but maybe add some comment that this may be deprecated in a future release?

I added a print to stderr and a comment.

webmiche · 2022-11-30T16:09:40Z

I updated this to a refactor of all dialects. Please give it another read (@math-fehr in particular).

math-fehr

Thanks! Yes, this is what I meant!
That looks good to me, I just added some nit comments.

xdsl/ir.py

math-fehr · 2022-12-01T17:09:55Z

xdsl/ir.py

+    def get_attributes(self) -> Iterator[Attribute]:
+        return iter(self.__attributes)
+
+    def __call__(self, ctx: MLContext) -> None:


I would say that we shouldn't keep it, especially since we do not have many users yet.
What do you think @georgebisbas ? You know more than us about these things.

xdsl/dialects/memref.py

xdsl/dialects/arith.py

xdsl/dialects/irdl.py

georgebisbas · 2022-12-01T18:08:52Z

xdsl/ir.py

+    def get_attributes(self) -> Iterator[Attribute]:
+        return iter(self.__attributes)
+
+    def __call__(self, ctx: MLContext) -> None:


Keeps compatibility for what exactly?

webmiche added the dialects Changes on the dialects label Nov 25, 2022

webmiche requested review from georgebisbas and math-fehr November 25, 2022 11:34

webmiche self-assigned this Nov 25, 2022

georgebisbas reviewed Nov 25, 2022

View reviewed changes

xdsl/xdsl_opt_main.py Outdated Show resolved Hide resolved

georgebisbas reviewed Nov 25, 2022

View reviewed changes

xdsl/ir.py Show resolved Hide resolved

georgebisbas reviewed Nov 25, 2022

View reviewed changes

superlopuh approved these changes Nov 26, 2022

View reviewed changes

xdsl/ir.py Show resolved Hide resolved

math-fehr approved these changes Nov 28, 2022

View reviewed changes

xdsl/ir.py Show resolved Hide resolved

webmiche commented Nov 28, 2022

View reviewed changes

xdsl/dialects/func.py Outdated Show resolved Hide resolved

webmiche commented Nov 28, 2022

View reviewed changes

xdsl/ir.py Outdated Show resolved Hide resolved

webmiche force-pushed the dialect_registration branch from 6bfb8a2 to 5ce990e Compare November 28, 2022 10:01

webmiche requested a review from math-fehr November 28, 2022 10:59

webmiche force-pushed the dialect_registration branch from 5ce990e to 5289865 Compare November 28, 2022 11:48

webmiche force-pushed the dialect_registration branch from 5289865 to 172286a Compare November 29, 2022 07:42

webmiche changed the title ~~compiler: Make dialect registration more intuitive~~ xDSL: Make dialect registration more intuitive Nov 29, 2022

webmiche force-pushed the dialect_registration branch from 172286a to ec74928 Compare November 30, 2022 14:05

webmiche linked an issue Nov 30, 2022 that may be closed by this pull request

Register dialects differently #205

Closed

webmiche commented Nov 30, 2022

View reviewed changes

math-fehr approved these changes Dec 1, 2022

View reviewed changes

georgebisbas reviewed Dec 1, 2022

View reviewed changes

webmiche force-pushed the dialect_registration branch 2 times, most recently from f50e38b to cdc3e8d Compare December 5, 2022 08:59

xDSL: make dialect registration more intuitive

cdc3e8d

webmiche merged commit 1cd6d79 into main Dec 5, 2022

webmiche mentioned this pull request Dec 5, 2022

xdsl: Use Annotated for type constraints for operation operands and results #210

Merged

webmiche deleted the dialect_registration branch December 14, 2022 12:33

xDSL: Make dialect registration more intuitive #231

xDSL: Make dialect registration more intuitive #231

Uh oh!

Conversation

webmiche commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webmiche commented Nov 25, 2022

Uh oh!

review-notebook-app bot commented Nov 25, 2022

Uh oh!

Uh oh!

Uh oh!

georgebisbas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

superlopuh commented Nov 26, 2022

Uh oh!

webmiche commented Nov 27, 2022

Uh oh!

math-fehr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

math-fehr commented Nov 28, 2022

Uh oh!

webmiche commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webmiche commented Nov 28, 2022

Uh oh!

math-fehr commented Nov 28, 2022

Uh oh!

webmiche commented Nov 28, 2022

Uh oh!

math-fehr commented Nov 28, 2022

Uh oh!

webmiche commented Nov 29, 2022

Uh oh!

codecov bot commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

webmiche commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgebisbas commented Nov 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmiche commented Nov 30, 2022

Uh oh!

math-fehr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

webmiche commented Nov 25, 2022 •

edited

Loading

webmiche commented Nov 28, 2022 •

edited

Loading

codecov bot commented Nov 29, 2022 •

edited

Loading

webmiche commented Nov 29, 2022 •

edited

Loading