This is an archive of the discontinued LLVM Phabricator instance.

[mlir][affine][transform] Simplify affine.min/max ops with given constraints
ClosedPublic

Authored by springerm on Jan 4 2023, 8:33 AM.

Details

Summary

This transform op uses mlir::simplifyConstrainedMinMaxOp to simplify affine.min and affine.max ops based on a given constraints.

Depends On: https://reviews.llvm.org/D141141

Diff Detail

Event Timeline

springerm created this revision.Jan 4 2023, 8:33 AM
springerm requested review of this revision.Jan 4 2023, 8:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache requested changes to this revision.Jan 6 2023, 6:11 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Affine/TransformOps/AffineTransformOps.td
52

this lacks a verifier that args are properly sized and invalid tests.

56

Why do you need this?

Is it practically too annoying to have with { (%1, %2, %3) confined to (0, 0, 0) - (16, 32, 64) } ?

mlir/lib/Dialect/Affine/TransformOps/AffineTransformOps.cpp
56

Why not applyToOne ?

64

llvm::zip will drop stuff, I think there is a recently added asserting version, try zip_equal

85

Seems like this should be a helper in FlatAffineValueConstraints ?

100

I think the typed form of the op gives you that in a uniform way ?

120

Have you considered the handle invalidation/staleness implications of using this, in particular in larger transforms?

This looks fishy to me ..

132

I'd rather have a slightly less nice syntax and drop all this C++.

If you really insist on nicer syntax, I'd want to see something more general and reusable like the subview custom printer/parser slightly adapted to this use case.

This revision now requires changes to proceed.Jan 6 2023, 6:11 AM
springerm updated this revision to Diff 486871.Jan 6 2023, 7:16 AM
springerm marked 8 inline comments as done.

address comments

mlir/include/mlir/Dialect/Affine/TransformOps/AffineTransformOps.td
56

New syntax: with [%0] within [0] and [32]. I wanted parentheses but couldn't figure out how to print the DenseI64ArrayAttr without []. Same for the -, it is not a valid literal, so changed to and.

mlir/lib/Dialect/Affine/TransformOps/AffineTransformOps.cpp
56

Doesn't work with multiple PDL_Operation operands (bounded_values is the problem).

85

I could add it in this change, but there is a minor inconsistency in the FlatAffineValueConstraints API that should be addressed first. Upper bounds are sometimes inclusive and sometimes exclusive. This needs to be fixed in FlatAffineValueConstraints and IntegerPolyhedron (and all users of the API).

Instead of adding another function that deals with UB, can we land this as is and add the helper together with the cleanup that fixes the API?

100

I checked with Alex and this requires adding a new transform handle type + relatively complex TableGen because we need to support !transform.op<"affine.min OR affine.max">. I could add it but not worth it IMO unless we really want to go properly typed everywhere.

120

Yes, I discussed this with Alex.

This op consumes (invalidates) the target handle and does not return anything. Also there is a check there is no overlap among target ops and the constrained ops, so we cannot come into a situation where a constrained op is erased while the transform is applied.

Note: This applyOpPatternsAndFold does not return LogicalResult, but a bool that indicates whether any IR was rewritten or not. The (void) cast does *not* discard error state here.

Updated the comment: Removed the We do not compose chained affine.min / affine.max ops part, which made no sense. affine.min and affine.max ops cannot be composed. (Ops of the same type could be but that is not good enough, we still need fixpoint iteration.)

springerm added inline comments.Jan 6 2023, 7:22 AM
mlir/lib/Dialect/Affine/TransformOps/AffineTransformOps.cpp
120

Actually, strict should be set to true here. But then the IR does not simplify as expected. Need to take a closer look at the GreedyPatternRewriter to see what's going on...

nicolasvasilache accepted this revision.Jan 6 2023, 7:35 AM

Thanks!
Please do get to the bottom of the rewriter stuff before landing.

This revision is now accepted and ready to land.Jan 6 2023, 7:35 AM

Thanks!
Please do get to the bottom of the rewriter stuff before landing.

Turns out this is a bug in the MultiOpPatternRewriteDriver. Fix in D141141.

springerm edited the summary of this revision. (Show Details)Jan 9 2023, 12:02 AM
springerm updated this revision to Diff 487304.Jan 9 2023, 12:05 AM
springerm edited the summary of this revision. (Show Details)

update

This revision was landed with ongoing or failed builds.Jan 13 2023, 1:29 AM
This revision was automatically updated to reflect the committed changes.