Page MenuHomePhabricator

[mlir][Affine] Remove single iteration affine.for ops in AffineLoopNormalize
ClosedPublic

Authored by dcaballe on Oct 27 2020, 2:21 PM.

Details

Summary

This patch renames AffineParallelNormalize to AffineLoopNormalize to make it
more generic and be able to hold more loop normalization transformations in
the future for affine.for and affine.parallel ops. Eventually, it could also be
extended to support scf.for and scf.parallel. As a starting point for affine.for,
the patch also adds support for removing single iteration affine.for ops to the
the pass.

Diff Detail

Event Timeline

dcaballe created this revision.Oct 27 2020, 2:21 PM
dcaballe requested review of this revision.Oct 27 2020, 2:21 PM
flaub added inline comments.Oct 27 2020, 4:29 PM
mlir/lib/Dialect/Affine/Transforms/AffineLoopNormalize.cpp
23

This needs to be defined in the mlir namespace (this was a bug I introduced originally).

97

This also needs to be defined in the mlir namespace:

void mlir::normalizeAffineFor(...
ftynse accepted this revision.Oct 29 2020, 8:37 AM
ftynse added inline comments.
mlir/lib/Dialect/Affine/Transforms/AffineLoopNormalize.cpp
97

This should only be defined in the mlir namespace if it was declared in it, which does not seem to be the case. For functions scoped in a translation unit, MLIR uses static instead of anonymous namespaces.

This revision is now accepted and ready to land.Oct 29 2020, 8:37 AM

Thanks for the review!

mlir/lib/Dialect/Affine/Transforms/AffineLoopNormalize.cpp
23

Ok, I'll fix it before committing it.

97

Ok, I'll make it static before committing. We can make it public once it implements all the functionality.

flaub added inline comments.Nov 2 2020, 6:26 PM
mlir/lib/Dialect/Affine/Transforms/AffineLoopNormalize.cpp
85

I'd prefer if this was part of the public interface so that I can use it in downstream projects. Shouldn't this then be in the mlir namespace?

dcaballe added inline comments.Nov 3 2020, 2:41 PM
mlir/lib/Dialect/Affine/Transforms/AffineLoopNormalize.cpp
85

I think if you want to use this as a utility you may probably want to invoke the promoteIfSingleIteration directly since it's the only thing this method is doing for now. If we make normalizeAffineFor public when it's actually not normalizing the loop bounds it's going to be a bit confusing for external users, I think.