This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Vector] Introduce UnrollVectorOptions to control vector unrolling.
ClosedPublic

Authored by mravishankar on Oct 19 2020, 2:45 PM.

Details

Summary

The current pattern for vector unrolling takes the native shape to
unroll to at pattern instantiation time, but the native shape might
defer based on the types of the operand. Introduce a
UnrollVectorOptions struct which allows for using a function that will
return the native shape based on the operation. Move other options of
unrolling like filterConstraints into this struct.

Diff Detail

Event Timeline

mravishankar created this revision.Oct 19 2020, 2:45 PM
mravishankar requested review of this revision.Oct 19 2020, 2:45 PM
aartbik requested changes to this revision.Oct 20 2020, 9:57 AM
aartbik added inline comments.
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
72

Drop the the, it's cleaner ;-)

73

VectorUnrollingOptions? (we put vector first in most other cases)

Also, perhaps describe that it provides a few functions up front, that would make reading this code easier.

Lastly, I am not sure what the style is but typically we put methods before fields. If possible can we avoid the using inside the struct, I find them easier to read in a separate section?

75

the word order in this sentence reads a bit hard.

If the following function returns failure...
or

A function that indicates whether vector unrolling should be attempted

93

This function sets the function as well, but it does not cause setNativeShapeFn to return anything
can you please rephrase?

130

vector unrolling expects ...

This revision now requires changes to proceed.Oct 20 2020, 9:57 AM
mravishankar marked 4 inline comments as done.

Address comments and rebase

mlir/include/mlir/Dialect/Vector/VectorTransforms.h
73

Its named UnrollVectorOptions since the pattern is UnrollVectorPattern. That seems more consistent.

I found hard to describe the struct without just enumerating what the fields are, and it seemed to better deecribe the fields directly.

The style here is similar to LinalgTilingOptions here

Did you upload the changes? Revision seems the same?
Also, please see the failure in the report.

mlir/include/mlir/Dialect/Vector/VectorTransforms.h
72

still there?

75

No change?

93

no change?

130

no change?

Upload patch.

@aartbik Sorry for the mishap. Updated the patch now.

Overall looks good, two more comments. Please find out what causes windows to fail (I am curious too what makes the CHECK different)

mlir/include/mlir/Dialect/Vector/VectorTransforms.h
109

Nit: you dont' set "setNativeShapeFn", but you set nativeShape, which causes setNativeShapeFn to return that value. I don't want to be overly pedantic, but since you are using function values here, wording this a bit better may help the future reader

141

single if, no {}

Address comments and rebase

mravishankar marked 7 inline comments as done.Oct 23 2020, 9:27 AM

I tried to repro the windows error, but not able to (took me a whole day to setup my windows work env). Hopefully rebase makes the problem go away.

Fix failing test (maybe)

aartbik accepted this revision.Oct 23 2020, 11:27 AM
This revision is now accepted and ready to land.Oct 23 2020, 11:27 AM
jurahul added inline comments.Oct 23 2020, 11:31 AM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
113

since tsShape is now constructed outside and captured by value, you can just return tsShape here now I think.

mravishankar added inline comments.Oct 23 2020, 12:54 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
113

Oh right. Let me try if that still passes :)