Page MenuHomePhabricator

[mlir] Convert `memref_reshape` to LLVM.
ClosedPublic

Authored by pifon2a on Oct 29 2020, 3:19 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Oct 29 2020, 3:19 AM
pifon2a requested review of this revision.Oct 29 2020, 3:19 AM
pifon2a updated this revision to Diff 301563.Oct 29 2020, 4:06 AM

remove print

ftynse requested changes to this revision.Oct 29 2020, 7:15 AM
ftynse added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2569

Nit: could we have a doc for this function?

2604–2605

Nit: would it be reasonable to have these things as additional accessors on UnrankedMemRefDescriptor?

2617–2620

I wonder if we cannot run into issues because of alignment properties on some weird architectures. The descriptor structure is not packed, so it is subject to alignment rules between elements as defined by LLVM's data layout. I don't have an example offhand, so it may be always fine, so I'd appreciate an argument why it is a safe thing to do in a comment. Otherwise, LLVMTypeConverter contains the llvm::DataLayout that we are targeting, which can be used to get proper offsets of elements in bytes and do all indexing arithmetic after bitcasting to i8*.

2688–2689

Nit: LLVM::LLVMType was fine here

2691

Please fix

2716–2717

Can't we use createIndexConstant instead here? Or at least not hardcode i32, I think there was some option controlling the bit size of the address arithmetic

2721–2726

Putting these addressing tricks (which are cool, I must admit!) into the UnrankedMemRefDescriptor sounds even more appealing to me. Since we seem to always need the triple allocated/aligned/store, we can have a function for those.

2747–2748

addArgument is invalid in conversion patterns, similarly to other in-place updates. rewriter.createBlock creates block with arguments, but you'd need to clone the remaining operations in there. Maybe splitBlock can be extended to also add arguments to the newly created block.

(I suspect the addArgument will most likely just work even if we rollback the change, but it may run into some bad use-def loop)

2771

(Beyond the scope): I wonder if we could later refactor this and the SCF-to-std lowering in a createLoop(function_ref bodyBuilder, function_ref conditionBuilder) that produces std control flow; the pattern infra will then lower the std ops into LLVM automatically.

This revision now requires changes to proceed.Oct 29 2020, 7:15 AM
frgossen accepted this revision.Oct 29 2020, 8:59 AM
pifon2a updated this revision to Diff 302382.Mon, Nov 2, 1:14 PM
pifon2a marked 8 inline comments as done.

Address all of the comments but one.

pifon2a added inline comments.Mon, Nov 2, 1:17 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2617–2620

I am not sure how many architectures, not to mention weird ones, are using unranked code generation. I would leave it like that for now and use llvm::DataLayout later if needed.

2721–2726

I added setters/getters to UnrankedMemRefDescriptor. At first I intended to have a separate PR that adds them, but it looks like the current PR is the best way to actually test them.

2771

Yes, that would be much more readable.

pifon2a added inline comments.Mon, Nov 2, 1:18 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2747–2748

extending splitBlock looks much harder than just using createBlock and cloning the remaining ops. I ll do that tomorrow.

ftynse added inline comments.Tue, Nov 3, 1:41 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2617–2620

Let's keep a TODO comment then. If we ever run into a problem, will be easier to debug.

pifon2a updated this revision to Diff 302523.Tue, Nov 3, 2:36 AM
pifon2a marked 2 inline comments as done.

Address the remaining comments.

ftynse accepted this revision.Tue, Nov 3, 2:38 AM
This revision is now accepted and ready to land.Tue, Nov 3, 2:38 AM
This revision was landed with ongoing or failed builds.Tue, Nov 3, 2:39 AM
This revision was automatically updated to reflect the committed changes.