This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [VectorOps] consolidate all vector utilities to one header/cc file
ClosedPublic

Authored by aartbik on Jan 28 2020, 4:47 PM.

Diff Detail

Event Timeline

aartbik created this revision.Jan 28 2020, 4:47 PM

Unit tests: pass. 62280 tests passed, 0 failed and 827 were skipped.

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 241237.Jan 29 2020, 11:31 AM

rebased with master

Unit tests: fail. 62306 tests passed, 1 failed and 838 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_recursive/try_lock_until.pass.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.

andydavis1 accepted this revision.Jan 29 2020, 2:14 PM

Great cleanup. The call sites look great. Thanks for doing this!

mlir/include/mlir/Dialect/VectorOps/VectorUtils.h
31

I think this could be named "computeStrides" as it could be applied to non-slices as well. It just takes a 'shape' and 'sizes'.

40

Since this is a library it might be good to make the name more verbose:

computeElementOffsetsFromVectorSliceOffsets

40

Given the target slice sizes of a vector...

47

nit: I prefer "computeSliceSizes", but good either way.

This revision is now accepted and ready to land.Jan 29 2020, 2:14 PM
aartbik updated this revision to Diff 241298.Jan 29 2020, 2:53 PM
aartbik marked 4 inline comments as done.

addressed comments

Thanks! I was not sure on the naming, and like your suggestions better indeed. All addressed.

dcaballe requested changes to this revision.Jan 29 2020, 2:58 PM
dcaballe added a subscriber: dcaballe.
dcaballe added inline comments.
mlir/include/mlir/Dialect/VectorOps/VectorUtils.h
1

Utils.h -> VectorUtils.h?

37

All these utilities returning SmallVector should be turned into void utility_name(..., SmallVectorImpl<element_type>& output_name) so that they can accept SmallVectors of arbitrary static sizes.

mlir/lib/Dialect/VectorOps/VectorTransforms.cpp
115

just curious... why is this static_cast needed?

This revision now requires changes to proceed.Jan 29 2020, 2:58 PM
aartbik marked 4 inline comments as done.Jan 29 2020, 3:13 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/VectorOps/VectorUtils.h
37

Note that this is a refactoring of existing code, so I would prefer to do that as follow up. But just in case you insist, can you point me to at least one occurrence of this elsewhere in the tree?

mlir/lib/Dialect/VectorOps/VectorTransforms.cpp
115

size_t vs int64_t (may cause some compilers to complain about sign diff in comparison)

aartbik updated this revision to Diff 241304.Jan 29 2020, 3:24 PM
aartbik marked an inline comment as done.

Utils -> VectorUtils typo

Unit tests: pass. 62309 tests passed, 0 failed and 838 were skipped.

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.

dcaballe accepted this revision.Jan 29 2020, 3:36 PM
dcaballe added inline comments.
mlir/include/mlir/Dialect/VectorOps/VectorUtils.h
37

Agree, we can address this separately.
There are a few examples here: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Analysis/LoopInfo.h#L256

mlir/lib/Dialect/VectorOps/VectorTransforms.cpp
115

Thanks! Sorry, I thought the cast applied to the result of the comparison. My bad.

This revision is now accepted and ready to land.Jan 29 2020, 3:36 PM
aartbik marked 3 inline comments as done.Jan 29 2020, 3:41 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/VectorOps/VectorUtils.h
37

Ah, I see what you mean. Thanks for pointing this out.

Unit tests: pass. 62310 tests passed, 0 failed and 838 were skipped.

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.

This revision was automatically updated to reflect the committed changes.
aartbik marked an inline comment as done.