This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add lowering for composite constant.
ClosedPublic

Authored by denis13 on Jan 20 2020, 3:00 AM.

Details

Summary

Add lowering for constant operation with ranked tensor type to
spv.constant with spv.array type.

Diff Detail

Event Timeline

denis13 created this revision.Jan 20 2020, 3:00 AM
denis13 updated this revision to Diff 239048.Jan 20 2020, 3:03 AM

Added a missing space in comment.

Unit tests: pass. 62009 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

Unit tests: pass. 62009 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

Harbormaster completed remote builds in B44382: Diff 239048.
mravishankar added inline comments.Jan 20 2020, 11:38 AM
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
194

Why not use RankedTensorType.

mlir/test/Conversion/StandardToSPIRV/std-to-spirv.mlir
225

Having the constant be a tensor<2x3xi32> while the result is a !spv.array<6 x i32> is weird. To a great extent this is an artifact of the way constants are stored in MLIR, but we should probably linearize the constant to be a tensor<6xi32> to make it less weird.

227

Same as above. We probably need to linearize the constant attribute to a tensor<6xi32>

mravishankar requested changes to this revision.Jan 20 2020, 11:38 AM
This revision now requires changes to proceed.Jan 20 2020, 11:38 AM
denis13 marked an inline comment as done.Jan 20 2020, 12:45 PM
denis13 added inline comments.
mlir/test/Conversion/StandardToSPIRV/std-to-spirv.mlir
225

@mravishankar thanks for review, I took those tests from structure-ops.mlir and linearize the type in same way as it works for memref, memref<2x3xf32> -> !spv.ptr<!spv.struct<!spv,array<6xf32>>>. I'll update according your suggestion. Thanks!

denis13 updated this revision to Diff 239189.Jan 20 2020, 12:51 PM

Addresses comments.

denis13 updated this revision to Diff 239191.Jan 20 2020, 12:53 PM

Update tests.

Unit tests: pass. 62028 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

Unit tests: pass. 62028 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

denis13 updated this revision to Diff 239347.Jan 21 2020, 9:23 AM

Remove check for func signature type converter introduced in D72999.

Unit tests: pass. 62028 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

antiagainst added inline comments.Jan 21 2020, 9:45 AM
mlir/test/Conversion/StandardToSPIRV/std-to-spirv.mlir
225

This does not seem to address Mahesh's comment about linearizing the tensor literal itself? (It should be easy given that the data blob remains the same; we just need to call DenseElementsAttr::reshape() on it.) We'll need a test case for n-D tensors. :)

@antiagainst @mravishankar oops, sorry for the misunderstanding

denis13 updated this revision to Diff 239378.Jan 21 2020, 10:54 AM

Linearize DenseElementsAttr.

Unit tests: pass. 62028 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

antiagainst accepted this revision.Jan 22 2020, 5:00 AM

Nice, thanks Denis for adding this!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 22 2020, 5:28 AM
This revision was automatically updated to reflect the committed changes.