This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Start a LinalgToStandard pass and move conversion to library calls.
ClosedPublic

Authored by nicolasvasilache on May 5 2020, 7:49 PM.

Details

Summary

This revision starts decoupling the include the kitchen sink behavior of Linalg to LLVM lowering by inserting a -convert-linalg-to-std pass.

The lowering of linalg ops to function calls was previously lowering to memref descriptors by having both linalg -> std and std -> LLVM patterns in the same rewrite.

When separating this step, a new issue occurred: the layout is automatically type-erased by this process. This revision therefore introduces memref casts to perform these type erasures explicitly. To connect everything end-to-end, the LLVM lowering of MemRefCastOp is relaxed because it is artificially more restricted than the op semantics. The op semantics already guarantee that source and target MemRefTypes are cast-compatible. An invalid lowering test now becomes valid and is removed.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 7:49 PM
bondhugula added inline comments.
mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp
352

Can LinAlgToLLVM now be completely removed? Do you still need anything in it? I couldn't immediately tell from the commit summary and this diff.

bondhugula added inline comments.May 5 2020, 9:12 PM
mlir/test/mlir-cpu-runner/linalg_integration_test.mlir
1

I see you still have all those reshape/slice/transpose being converted directly from linalg to llvm instead of via std.view/subview.

nicolasvasilache marked 4 inline comments as done.May 6 2020, 5:56 AM
nicolasvasilache added inline comments.
mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp
352

Not yet, I'm doing it in a staged fashion but the endgoal is indeed to remove it.

mlir/test/mlir-cpu-runner/linalg_integration_test.mlir
1

Yes for now this is still the case.

ftynse requested changes to this revision.May 7 2020, 1:01 AM

I think you need to relax the semantics of memref_cast first. It currently says (emphasis mine)

The memref_cast operation converts a memref from one type to an equivalent type with a compatible shape. The source and destination types are compatible if: a. both are ranked memref types with the same element type, affine mappings, address space, and rank but where the individual dimensions may add or remove constant dimensions from the memref type.

so the new uses introduced here and the their lowering directly contradict what is stated in the doc. (I am surprised we don't have a verifier to check it). I am not fundamentally opposed to such relaxation, but we may want to consider introducing a separate, more dangerous cast operation, similarly to static_cast/reinterpret_cast different in C++. The layout-changing cast may create out-of-bounds accesses whereas static/dynamic info cast will never do that.

mlir/lib/Conversion/LinalgToStandard/LinalgToStandard.cpp
21

Document this plz

47

result.resize(numLoops, IndexType::get(ctx)); spares you a loop

105

How about storing the result of b.create in a variable and then pushing it back? It will likely be less LoC and more readable at the same time.

107

I don't recall memref_cast, or its lowering, to support layout changes. It can add or remove static information, but a memref without an explicit layout map just has an implicit row-major layout. Did you mean to cast it to unranked memref maybe?

This revision now requires changes to proceed.May 7 2020, 1:01 AM
nicolasvasilache marked 2 inline comments as done.May 11 2020, 8:48 PM

so the new uses introduced here and the their lowering directly contradict what is stated in the doc.

Here, I fixed it :)

The `memref_cast` operation converts a memref from one type to an equivalent
type with a compatible shape. The source and destination types are
compatible if both are ranked memref types with the same element type,
address space, and rank but where the individual dimensions may add or
remove constant dimensions from the memref type.

More seriously, I think this is an oversight since the doc examples and the impl contradict the "same layout" part.

so the new uses introduced here and the their lowering directly contradict what is stated in the doc.

Here, I fixed it :)

The `memref_cast` operation converts a memref from one type to an equivalent
type with a compatible shape. The source and destination types are
compatible if both are ranked memref types with the same element type,
address space, and rank but where the individual dimensions may add or
remove constant dimensions from the memref type.

More seriously, I think this is an oversight since the doc examples and the impl contradict the "same layout" part.

This change still doesn't fit my mental model of casts (the model may be wrong, but it is likely an evidence of insufficient documentation). The change drops the layout unconditionally. Let's assume we cannot have a non-strided layout because all memrefs come from Linalg (may be worth an assert and/or a comment). When you cast from memref<4x8xf32, offset:0, strides: [1,4]> to memref<4x8xf32>, the strides practically become [8,1], which doesn't look desirable.

nicolasvasilache marked 5 inline comments as done.

Address review comments.

Update the MemRefCastOp doc.

@ftynse also updated the condition breakdown in the doc to match the verifier, lmk if that looks more reasonable.

Thanks!

Harbormaster completed remote builds in B56431: Diff 263430.
ftynse accepted this revision.May 12 2020, 8:45 AM

Makes senes now, thanks!

This revision was not accepted when it landed; it landed in state Needs Review.May 14 2020, 9:56 PM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.May 27 2020, 12:49 PM
mlir/include/mlir/Conversion/LinalgToStandard/LinalgToStandard.h
12

Can we trim this include?

mlir/include/mlir/Conversion/Passes.td
148

Should this come after SPIRV alphabetically?

@nicolasvasilache : did you address the post-commit review comments?

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 1:29 AM
Herald added a subscriber: msifontes. · View Herald Transcript