This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Add integration test for vector distribute transformation
ClosedPublic

Authored by ThomasRaoux on Oct 8 2020, 12:11 PM.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Oct 8 2020, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2020, 12:11 PM
ThomasRaoux requested review of this revision.Oct 8 2020, 12:11 PM
aartbik added inline comments.Oct 8 2020, 12:36 PM
mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir
5

I may be mistaken, but doesn't mlir_runner_utils link in mlir_c_runner_util as well? can you please check?

7

can you please add an empty line after the RUN section to be consistent with the other tests

14

just indent by 2

32

perhaps a few comments on what you test here

42

indent by 2

Address review comments.

ThomasRaoux marked 4 inline comments as done.Oct 8 2020, 1:06 PM
ThomasRaoux added inline comments.
mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir
5

Indeed. Removed the redundant mlir_c_runner_util.

aartbik accepted this revision.Oct 8 2020, 1:59 PM
aartbik added inline comments.
mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir
13

Note that the caller never deallocs this explicitly, perhaps add this too for pedantic accuracy

This revision is now accepted and ready to land.Oct 8 2020, 1:59 PM

Add missing deallocate

Thanks for the quick review, Aart!

mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir
14

Added the deallocs

and thank you for contributing to the integration tests!

LG thanks @ThomasRaoux

mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir
13

With the tensor constant suggestion, the alloc / dealloc would be properly inserted.

32

typo

41

With https://reviews.llvm.org/D88998 we can just create a tensor constant and lower it to memref.
This should simplify writing small integration tests.
Of course after a certain number of values it is more concise to write a loop.
Seems 64 is about right at the threshold :)

If you reduced that to say 5, the test could become tighter and more semantically meaningful.

OTOH there would still be a need for extracting the data from a tensor to a memref, so maybe this is not worth it and some abstractions are still missing.

Feel free to ignore, just wanted to surface the fact that the option exists now.