Page MenuHomePhabricator

[mlir] translate memref.reshape ops that have static shapes
ClosedPublic

Authored by ashay-github on May 5 2022, 1:49 PM.

Details

Summary

This patch references code for translating memref.reinterpret_cast ops
to add translation rules for memref.reshape ops that have a static shape
argument. Since reshape ops don't have offsets, sizes, or strides, this
patch simply sets the allocated and aligned pointers of the MemRef
descriptor.

Diff Detail

Event Timeline

ashay-github created this revision.May 5 2022, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 1:49 PM
ashay-github requested review of this revision.May 5 2022, 1:49 PM
ftynse requested changes to this revision.EditedMay 6 2022, 2:22 AM

Since reshape ops don't have offsets, sizes, or strides, this
patch simply sets the allocated and aligned pointers of the MemRef
descriptor.

The convention is that the descriptor always contains the correct size and stride information even if the lowerings may chose to ignore it and use the information statically available from types. The reason for this is the descriptor being also used to interface with C code that does not have access to the information containing in the memref type. Leaving size/stride information uninitialized will lead to UB in such code.

This revision now requires changes to proceed.May 6 2022, 2:22 AM

set stride, offset, and size attributes of MemRefDescriptor

The code now uses getStridesAndOffset() on the result MemRefType to infer the
stride, offset, and size attributes.

Thanks for the suggestion. I've updated the code and test to set the offset, stride, and size. PTAL.

I left some comments but I am also pretty new to this. Hope someone who's expert on this help to take a look.

mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1126

nit: could remove the auto successStrides and check if (failed(getStridesAndOffset...)) directly

1129

My understanding is that it's still possible to get non static offset and strides for static shape memref so need to check all static before calling desc.setConstantXXX.

check for static strides and offsets, otherwise return failure

ashay-github marked 2 inline comments as done.May 10 2022, 10:44 AM

Thanks @cathyzhyi! I've updated the code accordingly, but please let me know if I still got it wrong. Thanks!

cathyzhyi requested changes to this revision.May 10 2022, 3:12 PM
cathyzhyi added inline comments.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1132

Need to check for constant offset as well.

This revision now requires changes to proceed.May 10 2022, 3:12 PM

check for dynamic offset

Thanks @cathyzhyi! I added a check for !isStaticStrideOrOffset(offset) as well.

cathyzhyi accepted this revision.May 11 2022, 12:28 PM

LGTM! @ftynse could you help to double check because I am not super confident either.

ftynse accepted this revision.May 12 2022, 9:12 AM
ftynse added inline comments.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1123

Nit: do not specify the number of stack elements in SmallVector unless you have a strong reason to use a specific number.

This revision is now accepted and ready to land.May 12 2022, 9:12 AM

dropping element count in SmallVector usage

ashay-github marked 2 inline comments as done.May 12 2022, 10:34 AM