Page MenuHomePhabricator

[mlir][vector] Update doc strings for insert_map/extract_map and fix insert_map semantic
Needs ReviewPublic

Authored by ThomasRaoux on Fri, Oct 16, 10:00 AM.

Details

Summary

Based on discourse discussion, fix the doc string and remove examples with wrong semantic. Also fix insert_map semantic by adding missing operand for vector we are inserting into.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Fri, Oct 16, 10:00 AM
ThomasRaoux requested review of this revision.Fri, Oct 16, 10:00 AM

Thanks Thomas, this is a better separation of op semantics from transformation considerations indeed!

This revision is now accepted and ready to land.Fri, Oct 16, 12:06 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
463–464

Actually let's simplify this too:

Takes an 1-D vector and extract a sub-part of the vector starting at `id` with
a size of `vector size / multiplicity`.

No need to speculate on all the things that %id will represent in the op definition.
This makes it more in line with the LLVM boundary case: https://llvm.org/docs/LangRef.html#extractelement-instruction.

686–687

Same here please.

Thanks!

aartbik added inline comments.Fri, Oct 16, 2:25 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
463–464

nit: Takes ... and extracts (same verb form)

mlir/include/mlir/Dialect/Vector/VectorOps.td
707

Note this is missing the vector to insert into.
Syntax should also be symmetric to the extract counterpart.

%n = constant dense<x> : vector<4xi32>
%3 = vector.insert_map %1, %n[%idx0: 2] : vector<2xi32> to vector<4xi32>
nicolasvasilache requested changes to this revision.Sat, Oct 17, 11:48 PM
This revision now requires changes to proceed.Sat, Oct 17, 11:48 PM
ThomasRaoux retitled this revision from [mlir][vector] Update doc strings for insert_map/extract_map to [mlir][vector] Update doc strings for insert_map/extract_map and fix insert_map semantic.
ThomasRaoux edited the summary of this revision. (Show Details)
ThomasRaoux marked 3 inline comments as done.
ThomasRaoux added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
707

Makes sense, I think that was the key missing part.

mehdi_amini added inline comments.Mon, Oct 19, 2:59 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
707

Ah that changes quite the picture!

But why do we need a multiplicity attribute now? We have the type of the vector we insert into: don't we need only the index?

ThomasRaoux added inline comments.Mon, Oct 19, 4:12 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
707

Good point, this is redundant right now. I think there was some ideas behind it to be able to handle non aligned cases but this is definitely premature at this point. Removing it to make things clearer for now.

Remove multiplicity attribute for now as it is redundant.