This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Lower linalg.reshape to LLVM for the static case
ClosedPublic

Authored by nicolasvasilache on Jan 6 2020, 10:10 PM.

Details

Summary

This diff adds lowering of the linalg.reshape op to LLVM.

A new descriptor is created with fields initialized as follows:

  1. allocatedPTr, alignedPtr and offset are copied from the source descriptor
  2. sizes are copied from the static destination shape
  3. strides are copied from the static strides collected with getStridesAndOffset

Only the static case in which the target view conforms to strided memref
semantics is supported. Other cases are left for future work and will be added on
a per-need basis.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 10:10 PM

Remove debug spew.

Unit tests: pass. 61273 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61273 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ftynse marked an inline comment as done.Jan 7 2020, 2:02 PM
ftynse added inline comments.
mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp
179

I wonder whether hasStaticShape is the right name here. It only checks the sizes, but not the strides, which is a bit counter-intuitive. Not for this commit certainly...

189

Can reshape do permutations using affine maps? It does not seem supported here, neither it is guarded against in the matching.

198

MemRefDescriptor has setConstantSize for this purposes, consider replicating it in BaseViewConversionHelper ?

nicolasvasilache marked 4 inline comments as done.

Address review comments.

mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp
179

I am not sure I follow. If the destination does not have static shape we can early exit and no need to compute the strides.
If it does, we still need to check that it has static strides, which happens below.

189

No, reshape can only collapse contiguous dims and this is enforced in the op verifier.

Unit tests: pass. 61305 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ftynse accepted this revision.Jan 8 2020, 2:26 AM
ftynse marked an inline comment as done.
ftynse added inline comments.
mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp
179

I find it confusing that strides are not included in "shape", but it's a separate discussion.

This revision is now accepted and ready to land.Jan 8 2020, 2:26 AM
This revision was automatically updated to reflect the committed changes.