Skip to content

Conversation

@3ulalia
Copy link

@3ulalia 3ulalia commented Apr 8, 2025

Currently, LoadOperation and StoreOperation expect the instructions to just be "load" and "store", respectively. This is not possible in current MLIR - indeed, I don't know if it was ever possible. Regardless, since the operations involve MemRef types anyway, this isn't a significant change, and just makes it compliant with modern MLIR.

(fixes the original cause of #42)

Fixes #42.

@3ulalia
Copy link
Author

3ulalia commented Apr 10, 2025

@tbennun Is there anything preventing this from being merged? It doesn't solve #42's root problem, but it does bring pymlir in line with modern MLIR.

@tbennun
Copy link
Contributor

tbennun commented Apr 10, 2025

@3ulalia yes, this file is entirely outdated with its "standard" dialect, which doesn't exist anymore. There are many ops here that are no longer written this way (for example, arith.*). I anticipate other issues in the future if this is not addressed.

@tbennun
Copy link
Contributor

tbennun commented Apr 10, 2025

@3ulalia The least I would ask is to move this to a separate memref.py file, just like we did with func. Thank you!

@3ulalia
Copy link
Author

3ulalia commented Apr 10, 2025

@tbennun Done; testing on the once-problematic input reveals that it works perfectly.

@3ulalia
Copy link
Author

3ulalia commented Apr 10, 2025

There are rather a lot of operations in standard.py that should be re-namespaced into memref or arith; since I expect to use those quite regularly I'll go through and fix them at some point. For now, this should suffice as a stopgap/skeletal change.

Currently, LoadOperation and StoreOperation expect the instructions to
just be "load" and "store", respectively. This is not possible in
current MLIR - indeed, I don't know if it was ever possible. Regardless,
since the operations involve MemRef types anyway, this isn't a
significant change, and just makes it compliant with modern MLIR.

Specifically, this change adds the MemRef dialect, whose currently
supported operations are `load` and `store`. (more to come!)
@3ulalia
Copy link
Author

3ulalia commented Apr 10, 2025

Whoops, forgot to remove the old classes from standard.py. Also mistyped a couple of fields in memref.py. @tbennun should be good to merge now.

@3ulalia
Copy link
Author

3ulalia commented Apr 15, 2025

@tbennun bump; is there anything else that needs to be changed here?

@tbennun
Copy link
Contributor

tbennun commented Apr 30, 2025

@3ulalia it looks good! We are failing a few tests though.

@3ulalia
Copy link
Author

3ulalia commented May 6, 2025

Ah, right, that would be because we need to update the test suites to make them compatible. Will do sometime this week.

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.

Improper parsing of memref syntax

2 participants