After the MemRef has been split out of the Standard dialect, the
conversion to the LLVM dialect remained as a huge monolithic pass.
This is undesirable for the same complexity management reasons as having
a huge Standard dialect itself, and is even more confusing given the
existence of a separate dialect. Extract the conversion of the MemRef
dialect operations to LLVM into a separate library and a separate
conversion pass.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
A big change, so you probably want people from all over to review. I looked at the vector and sparse changes, and they LGTM.
Thanks for doing this refactoring, that must have been a bit of a pain ;-)
The change is pretty mechanical, mostly moving code around and making sure the new pass is called whenever relevant. I don't expect a deep review here.
Yeah, I mostly meant getting a review from all the many different component owners, not necessarily a very deep one.
Thanks for doing this! I did not deeply review all the tests and code, as I assume it was mostly copied/moved. Please fix the few minor things I found.
mlir/include/mlir/Conversion/Passes.td | ||
---|---|---|
262 | It seems to be a common theme but why are all the passes module passes here? Is there a technical reason? No need to change it as part of this refactoring. | |
mlir/lib/Conversion/MemRefToLLVM/AllocLikeConversion.cpp | ||
2 | This is missing the comment header. | |
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp | ||
511 | Nit: Extra space. | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
1240 ↗ | (On Diff #357257) | Debugging left-over? |
mlir/test/Conversion/MemRefToLLVM/convert-static-memref-ops.mlir | ||
4 | Is this useful? |
Address review
mlir/include/mlir/Conversion/Passes.td | ||
---|---|---|
262 | I don't know about the other passes, memref-to-llvm needs to be a module pass because it rewrites memref.global operations are module-level symbols. Function pass wouldn't even see those because they are outside of functions, and there are also concurrency concerns. | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
1240 ↗ | (On Diff #357257) | Thanks! |
It seems to be a common theme but why are all the passes module passes here? Is there a technical reason?
No need to change it as part of this refactoring.