- Add support to vectorize induction variables of loops that are not mapped to any vector dimension in SuperVectorize pass.
- Fix a bug in getForInductionVarOwner.
Details
Diff Detail
Event Timeline
Thanks for adding support for this missing case, Amy! The implementation looks great! I only think we shouldn't use a special case for uniform IVs, they are just like any other uniform value. I would extend the uniform utilities to support this case instead of adding a new one. More details inline. Other than that, it looks great!
Thanks!
Diego
mlir/lib/Dialect/Affine/Transforms/SuperVectorize.cpp | ||
---|---|---|
261 | Instead of adding a new bullet, I would extend the previous ones about "Uniform operands". Uniform IVs are just uniform values. | |
1106 | I think this comment wouldn't be necessary. Uniform IVs are just uniform values so they would fall into #3. It just happen that some cases are missing in the uniform check, which is what you are adding here. | |
1124 | I think the vectorizeIV utility shouldn't be needed. We should be able to use vectorizeUniform for this case. | |
1150 | I think we should move this check into isUniformDefinition. There shouldn't be any difference between vectorizing a uniform IV and any other uniform value. Actually, if the IV is outside the vectorize loop(s), it should be hitting that case already (line 1161), right? This extra check would handle uniform IVs inside the vectorized loop(s). Am I missing something here? | |
mlir/test/Dialect/Affine/SuperVectorize/vectorize_1d.mlir | ||
198 | nit: we can drop %{{.*}} = | |
200 | Since you are capturing the IVs, it would interesting to check that the transfer read is actually using the scalar version of the IVs | |
211–217 | Since this is only testing the vectorization of IVs, could we simplify the test and remove all the ops not related to the IVs, including iter_args? |
Address review comments to extend the uniform utilities to support the induction variable vectorization.
mlir/lib/Dialect/Affine/Transforms/SuperVectorize.cpp | ||
---|---|---|
1150 | Yes, you're right. I have moved this check into isUniformDefinition. | |
mlir/test/Dialect/Affine/SuperVectorize/vectorize_1d.mlir | ||
200 | This transfer read is using the result of affine.apply. I have added checks to see if affine.apply is using the scalar version of the IVs. |
@dcaballe Thank you for the review. We'll merge it tomorrow if there are no more comments.
Instead of adding a new bullet, I would extend the previous ones about "Uniform operands". Uniform IVs are just uniform values.