This is an archive of the discontinued LLVM Phabricator instance.

[mlir] materialize strided memref layout as attribute
ClosedPublic

Authored by ftynse on Aug 29 2022, 9:28 AM.

Details

Summary

Introduce a new attribute to represent the strided memref layout. Strided
layouts are omnipresent in code generation flows and are the only kind of
layouts produced and supported by a half of operation in the memref dialect
(view-related, shape-related). However, they are internally represented as
affine maps that require a somewhat fragile extraction of the strides from the
linear form that also comes with an overhead. Furthermore, textual
representation of strided layouts as affine maps is difficult to read: compare
affine_map<(d0, d1, d2)[s0, s1] -> (d0*32 + d1*s0 + s1 + d2)> with
strides: [32, ?, 1], offset: ?. While a rudimentary support for parsing a
syntactically sugared version of the strided layout has existed in the codebase
for a long time, it does not go as far as this commit to make the strided
layout a first-class attribute in the IR.

This introduces the attribute and updates the tests that using the pre-existing
sugared form to use the new attribute instead. Most memref created
programmatically, e.g., in passes, still use the affine form with further
extraction of strides and will be updated separately.

Update and clean-up the memref type documentation that has gotten stale and has
been referring to the details of affine map composition that are long gone.

See https://discourse.llvm.org/t/rfc-materialize-strided-memref-layout-as-an-attribute/64211.

Diff Detail

Event Timeline

ftynse created this revision.Aug 29 2022, 9:28 AM
ftynse requested review of this revision.Aug 29 2022, 9:28 AM
Herald added a project: Restricted Project. · View Herald Transcript

Downstreams can update the mechanical part of the tests using git grep 'offset: ' test * -r -l | xargs sed -i -e 's/offset: \([0-9?]\+\), strides: \[\([0-9, ?]*\)\]/strided<[\2], offset: \1>/g'

Ship it with fire!

mlir/include/mlir/IR/BuiltinTypes.td
371

nit, missing right '>'

mlir/test/Conversion/MemRefToLLVM/memref-to-llvm.mlir
612

I see here and below that memref.transpose produce a memref with an affine map instead of the new attr.

Given the clear explanation in the commit message:

Most memref created
programmatically, e.g., in passes, still use the affine form with further
extraction of strides and will be updated separately.

I am fine to pick that up as a followup.

mlir/test/Dialect/Linalg/tile.mlir
44

same for memref.subview, fine as a followup

This revision is now accepted and ready to land.Aug 30 2022, 12:25 AM
bondhugula added inline comments.
mlir/include/mlir/IR/BuiltinAttributes.h
1083

A stride cannot take value 0 -> A stride is always positive?

mlir/include/mlir/IR/BuiltinAttributes.td
989

a list of strides along each dimension -> a list of strides, one for each dimension

997

Mention about strides being positive and offset being non-negative?

bondhugula added inline comments.
mlir/include/mlir/IR/BuiltinTypes.td
312

that indicates the how the indices -> that indicates how indices

342

including

ftynse updated this revision to Diff 456683.Aug 30 2022, 8:18 AM
ftynse marked 8 inline comments as done.

Address reviews.

This revision was landed with ongoing or failed builds.Aug 30 2022, 8:20 AM
This revision was automatically updated to reflect the committed changes.

Looks like this caused a failure on the windows mlir bot: https://lab.llvm.org/buildbot/#/builders/13/builds/25175

Thanks for the note, for some reason, I haven't received the automated email. Fixed in 0fd95808650aa2addc851ba66a6fc00a83f3f28f.

mlir/include/mlir/IR/BuiltinAttributes.td
997

Good point!

mlir/include/mlir/IR/BuiltinAttributes.td
997

Nope: strides and offsets may be negative, which can be used to encode "loop reversal"-like layout in the type.
This is not actively used but there is no restriction beyond "no internal aliasing".
This is also why the sentinel or ? is -max_int and not -1

ftynse marked an inline comment as done.Aug 31 2022, 7:49 AM
ftynse added inline comments.
mlir/include/mlir/IR/BuiltinAttributes.td
997

The verifier enforces otherwise because this has not been spec'd. There was also no test breakage because of that, so we don't seem to be using this. We can revise when there is a need.