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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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... 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 | |
130 | vector unrolling expects ... |
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 |
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 {} |
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.
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. |
mlir/include/mlir/Dialect/Vector/VectorTransforms.h | ||
---|---|---|
113 | Oh right. Let me try if that still passes :) |
Drop the the, it's cleaner ;-)