Page MenuHomePhabricator

[mlir] [VectorOps] Add expand/compress operations to Vector dialect
ClosedPublic

Authored by aartbik on Wed, Jul 29, 2:13 PM.

Details

Summary

Introduces the expand and compress operations to the Vector dialect
(important memory operations for sparse computations), together
with a first reference implementation that lowers to the LLVM IR
dialect to enable running on CPU (and other targets that support
the corresponding LLVM IR intrinsics).

Diff Detail

Event Timeline

aartbik created this revision.Wed, Jul 29, 2:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
aartbik requested review of this revision.Wed, Jul 29, 2:13 PM
aartbik updated this revision to Diff 282299.Fri, Jul 31, 12:52 PM

fixed typos

reidtatge added inline comments.Tue, Aug 4, 10:36 AM
mlir/integration_test/Dialect/Vector/CPU/test-expand.mlir
43

I'm not sure what the MLIR team philosophy is on this, but in test code that creates test inputs (like everything up to this point in this case), I find myself carefully reading that code to make sure I understand what input test values are expected to be, which seems to be besides the point. Would it make sense to print these values and //CHECK them, so that reviewers don't have to figure out what the inputs are, and that they are correct? (And it serves as additional testing for the "boilerplate" stuff, just in case...

reidtatge accepted this revision.Tue, Aug 4, 10:36 AM
This revision is now accepted and ready to land.Tue, Aug 4, 10:36 AM
aartbik marked an inline comment as done.Tue, Aug 4, 11:02 AM
aartbik added inline comments.
mlir/integration_test/Dialect/Vector/CPU/test-expand.mlir
43

Good question. I sometimes print the test values also (see some other tests), but indeed not in this case. I am not sure we have an established philosophy yet, but in the long run, I am thinking of replacing setup code like this with something simpler, like
%all = [1,...,1]
or something like that (perhaps read from string), just for readability.

aartbik marked an inline comment as done.Tue, Aug 4, 11:27 AM

Ai, I need to rebase/adjust my tests with the new llvm type format that Alex just checked in. Stay tuned.....

aartbik updated this revision to Diff 282979.Tue, Aug 4, 11:37 AM

rebased with new llvm type syntax