This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Arith] ValueBoundsOpInterface: Reify with Arith ops
ClosedPublic

Authored by springerm on Mar 21 2023, 6:34 AM.

Details

Summary

This revision adds an additional reifyValueBounds helper that reifies the IR with Arith ops instead of Affine ops. This is needed to support value bounds for integer types different from index in a subsequent revision.

Depends On: D147987

Diff Detail

Event Timeline

springerm created this revision.Mar 21 2023, 6:34 AM
springerm requested review of this revision.Mar 21 2023, 6:34 AM
dcaballe added inline comments.Apr 6 2023, 4:28 PM
mlir/include/mlir/Dialect/Arith/Transforms/Transforms.h
37

I wonder if we could we make the non-index / index type output transparent to the user, i.e., we could have a single interface and then two internal implementations? Or even a single arith-based implementation and then try to fold ops into maps? Not sure if we have any utility that could help with that.

Otherwise, I think at least we should be more specific about the names... something like reifyIndexValueBound and reifyNonIndexValueBound?

springerm added inline comments.Apr 7 2023, 1:26 AM
mlir/include/mlir/Dialect/Arith/Transforms/Transforms.h
37

we could have a single interface and then two internal implementations
Not sure what this means. We have a single op interface with two methods (ValueBoundsOpInterface::populateBoundsForIndexValue and ValueBoundsOpInterface::populateBoundsForShapeValueDim).

try to fold ops into maps
What kind of maps you mean? Affine maps?

dcaballe added inline comments.Apr 7 2023, 10:48 AM
mlir/include/mlir/Dialect/Arith/Transforms/Transforms.h
37

(s/interface/API/ in my comment). We have affine::reifyValueBound and arith::reifyValueBound, right? I'm asking if we should unify these two methods into one (if generating affine maps or arith ops depends on the type of operations analized) or, otherwise, use more specific names for the APIs that clearly state the difference between the arith and the affine implementation. We should also elaborate about the differences between the two in the doc. At this point, it's not easy to understand the difference.

try to fold ops into maps
What kind of maps you mean? Affine maps?

This comment was before seeing the follow-up patches. I know see that you are decomposing the resulting affine map when needed.

springerm updated this revision to Diff 512317.Apr 10 2023, 7:49 PM

address comments

springerm marked 2 inline comments as done.Apr 10 2023, 7:57 PM
springerm added inline comments.
mlir/include/mlir/Dialect/Arith/Transforms/Transforms.h
37

Otherwise, I think at least we should be more specific about the names... something like reifyIndexValueBound and reifyNonIndexValueBound?

I split the function in D147987.

We have affine::reifyValueBound and arith::reifyValueBound, right? I'm asking if we should unify these two methods into one (if generating affine maps or arith ops depends on the type of operations analized) or, otherwise, use more specific names for the APIs that clearly state the difference between the arith and the affine implementation.

The kind of operations that are generated depend on which function is called. arith::... generates Arith dialect ops. affine::... generates Affine dialect ops. Users of arith::... do not have to depend on the Affine dialect. (And vice versa.) I think the namespace should be enough to tell the two apart.

Note: The only thing that these functions do is materializing a computed bound in IR. Both call the same ValueBoundsConstraintSet::computeBound helper function.

dcaballe accepted this revision.Apr 18 2023, 12:18 AM
dcaballe added inline comments.
mlir/include/mlir/Dialect/Arith/Transforms/Transforms.h
37

Good point. I hadn't thought about dependences...

No strong opinion but there is no harm in making the name a bit more explicit. The following code from below:

  reified = arith::reifyShapedValueDimBound(
      rewriter, op->getLoc(), *boundType, value, *dim, stopCondition);
} else {
  reified = reifyShapedValueDimBound(
      rewriter, op->getLoc(), *boundType, value, *dim, stopCondition);

would be confusing for someone without too much context on the implementation.

This revision is now accepted and ready to land.Apr 18 2023, 12:18 AM
springerm marked an inline comment as done.Apr 18 2023, 12:49 AM
springerm added inline comments.
mlir/include/mlir/Dialect/Arith/Transforms/Transforms.h
37

This will read better once the Affine dialect is wrapped in a namespace affine {} like all the other dialects. Then we will have arith::reifyShapedValueDimBound and affine::reifyShapedValueDimBound.

This revision was landed with ongoing or failed builds.Apr 18 2023, 12:55 AM
This revision was automatically updated to reflect the committed changes.
springerm marked an inline comment as done.