This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [VectorOps] fixed bug in vector.insert_strided_slice lowering
ClosedPublic

Authored by aartbik on Jan 30 2020, 11:30 AM.

Details

Summary

Rationale:
When lowering to LLVM for different rank insert (n vs k), the offset
arrays needs to drop one dimension (becomes n-1), but the strides
array needs to be preserved (remains k). With regression test.
Note that this example was actually in the documentation, so
extra important to do it right :-)

Diff Detail

Event Timeline

aartbik created this revision.Jan 30 2020, 11:30 AM

Unit tests: fail. 62352 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 62352 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 62352 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 62352 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

aartbik updated this revision to Diff 241597.Jan 30 2020, 3:07 PM

sync back with latest pulled master

Unit tests: fail. 62343 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Jan 31 2020, 5:37 AM

LGTM after changing the test to capture %arg0 and %arg1. Thanks!

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
7

Nit: these changes to the test look orthogonal to the commit

551

Let's capture SSA names for arguments as well:

CHECK-LABEL: llvm.func @insert_strided_slice3
CHECK-SAME: %[[arg0:.*]]: !llvm<
CHECK-SAME: %[[arg1:.*]]: !llvm<

(CHECK-LABEL does not support capture)

This revision is now accepted and ready to land.Jan 31 2020, 5:37 AM
aartbik updated this revision to Diff 241774.Jan 31 2020, 11:00 AM
aartbik marked 4 inline comments as done.

args in patterns

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
7

They are, but most teams allow minor drive-by comments, I hope that is okay here too.
In this case, I was a bit triggered by the different formats for the LABEL check ;-)

551

Also did the other occurrences for consistency.

Unit tests: fail. 62343 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Thanks for fixing this Aart!

This revision was automatically updated to reflect the committed changes.