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

[springerm]

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

[springerm]

rebase

[dcaballe]

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]

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]

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]

address comments

[springerm]

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]

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.

[springerm]

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.