This is an archive of the discontinued LLVM Phabricator instance.

[mlir] factor memref-to-llvm lowering out of std-to-llvm
ClosedPublic

Authored by ftynse on Jul 8 2021, 5:45 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ftynse created this revision.Jul 8 2021, 5:45 AM
ftynse requested review of this revision.Jul 8 2021, 5:45 AM
Herald added a project: Restricted Project. · View Herald Transcript

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 ;-)

ftynse added a comment.Jul 8 2021, 9:26 AM

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.

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.

ftynse updated this revision to Diff 357257.Jul 8 2021, 9:38 AM

Headers

silvas accepted this revision.Jul 8 2021, 10:10 AM
This revision is now accepted and ready to land.Jul 8 2021, 10:10 AM
herhut accepted this revision.Jul 8 2021, 10:10 AM

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?

ftynse updated this revision to Diff 357450.Jul 9 2021, 2:37 AM
ftynse marked 5 inline comments as done.

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!

ftynse updated this revision to Diff 357483.Jul 9 2021, 4:56 AM

Attempt to appease the pattern order in Windows

This revision was automatically updated to reflect the committed changes.
mlir/test/Conversion/StandardToLLVM/convert-argattrs.mlir