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

core: Implement MemrefLayoutAttr base class and use in parser. #2718

Merged
merged 6 commits into from
Jun 13, 2024

Conversation

PapyChacal
Copy link
Collaborator

@PapyChacal PapyChacal commented Jun 13, 2024

This is a barebone mimic of the upstream MemrefLayoutAttrInterface, as it does not implement any of the methods; only mark attributes as acceptable by the parser. This solve the parser problem though, and allow the same extensibility on accepted layouts.

The uniplemented methods were not implemented on the existing layout attributes anyway, so I'm in favor of just merging, moving on, and think about implementing them in an interface style later.

Support AffineMaps in layout :D But also opens up to mark other attributes as acceptable as memref layouts. Interface methods left for future work for now!
@PapyChacal PapyChacal added the core xDSL core (ir, textual format, ...) label Jun 13, 2024
@PapyChacal PapyChacal requested a review from jorendumoulin June 13, 2024 11:15
@PapyChacal PapyChacal self-assigned this Jun 13, 2024
PapyChacal and others added 2 commits June 13, 2024 12:21
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
@PapyChacal PapyChacal marked this pull request as ready for review June 13, 2024 11:23
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (82a6ad6) to head (9c6e004).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2718   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files         367      367           
  Lines       47465    47468    +3     
  Branches     7234     7234           
=======================================
+ Hits        42654    42659    +5     
+ Misses       3698     3697    -1     
+ Partials     1113     1112    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@jorendumoulin jorendumoulin left a comment

Choose a reason for hiding this comment

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

I have tested this on the snax side, and everything works beautifully. Thanks a lot for your help on this!

Somewhat unrelated: the changes mix the use of isinstance and isa. When should one use which?

@superlopuh
Copy link
Member

isinstance is a built-in Python thing, and I would say is the recommended thing to use when it's sufficient. isa is an xDSL function, that can handle more things, notably Python generics and PyRDL constraints.

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this!
I do remember not implementing it properly, and I do really HATE what MLIR is doing here, but that's life.
It is one of the rare case in MLIR where parsing depends on an interface, which means here that if you want to use a memory layout that is unregistered, then the parsing will fail even though we may want it to work (Just saying this so you know in case you encounter this).

@PapyChacal
Copy link
Collaborator Author

Thanks for implementing this! I do remember not implementing it properly, and I do really HATE what MLIR is doing here, but that's life. It is one of the rare case in MLIR where parsing depends on an interface, which means here that if you want to use a memory layout that is unregistered, then the parsing will fail even though we may want it to work (Just saying this so you know in case you encounter this).

Good point; but how do you know if your unregistered attribute is a memory space or layout then?

@PapyChacal PapyChacal merged commit 168758f into main Jun 13, 2024
10 checks passed
@PapyChacal PapyChacal deleted the emilien/memreflayoutattr branch June 13, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core xDSL core (ir, textual format, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants