-
Notifications
You must be signed in to change notification settings - Fork 48
Change LoadOperation and StoreOperation to use MemRef #43
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
base: master
Are you sure you want to change the base?
Conversation
|
@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. |
|
@3ulalia The least I would ask is to move this to a separate |
|
@tbennun Done; testing on the once-problematic input reveals that it works perfectly. |
|
There are rather a lot of operations in |
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!)
|
Whoops, forgot to remove the old classes from |
|
@tbennun bump; is there anything else that needs to be changed here? |
|
@3ulalia it looks good! We are failing a few tests though. |
|
Ah, right, that would be because we need to update the test suites to make them compatible. Will do sometime this week. |
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.