Page MenuHomePhabricator

[mlir][shape] Start a pass that lowers shape constraints.
ClosedPublic

Authored by silvas on Sep 18 2020, 1:58 PM.

Details

Summary

This pass converts shape.cstr_* ops to eager (side-effecting)
error-handling code. After that conversion is done, the witnesses are
trivially satisfied and are replaced with shape.const_witness true.

We also throw in a targeted set of canonicalization patterns to clean up
trivial shape.assuming (we can add more as needed; we don't want users
of this pass to always have to run a full canonicalization after this
pass).

Diff Detail

Event Timeline

silvas created this revision.Sep 18 2020, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 1:58 PM
silvas requested review of this revision.Sep 18 2020, 1:58 PM
silvas updated this revision to Diff 292895.Sep 18 2020, 2:00 PM

Fix variable name.

Harbormaster completed remote builds in B72240: Diff 292895.

Nice!

mlir/lib/Conversion/ShapeToStandard/ConvertShapeConstraints.cpp
47

Why not use SelectOp?

124

Is this meant as an optimization to avoid running all canonicalization patterns? Otherwise, it is not clear to me what the benefits over just canonicalizing is.

silvas added inline comments.Sep 21 2020, 12:24 PM
mlir/lib/Conversion/ShapeToStandard/ConvertShapeConstraints.cpp
47

I was being consistent with ConvertShapeToStandard's lowering of broadcast, which also uses an IfOp

124

Correct. Canonicalizing the whole program is overkill and will lead to fragile interaction of this pass with other passes.

herhut added inline comments.Sep 22 2020, 1:30 AM
mlir/lib/Conversion/ShapeToStandard/ConvertShapeConstraints.cpp
47

I think both should use SelectOp :)

124

I am worried about the _fragile interaction_ part. Canonicalizing should always be ok and dropping it an canonicalizer pass should not break other passes. Hiding that here seems wrong to me.

Also, users of this now have no choice, as they always get the whole package. I'd much rather just get the constraint lowering patterns. Could this optimization be moved to the use site?

For test, just using --canonicalize won't hurt.

tpopp added inline comments.Sep 22 2020, 6:34 AM
mlir/include/mlir/Conversion/ShapeToStandard/ShapeToStandard.h
19

Alphabetical order.

tpopp resigned from this revision.Sep 22 2020, 6:34 AM
jpienaar added inline comments.Sep 22 2020, 7:28 AM
mlir/lib/Conversion/ShapeToStandard/ConvertShapeConstraints.cpp
2

This doesn't seem to be standard form here

66

Isn't this more that this generates code to ... ? Meaning, if we just wanted to compare we'd use the helper function here

111

Let's add a comment here describing why this works

silvas updated this revision to Diff 293511.Sep 22 2020, 11:08 AM
silvas marked 3 inline comments as done.

Address comments

mlir/lib/Conversion/ShapeToStandard/ConvertShapeConstraints.cpp
47

will convert both in a follow-up.

66

Done.

111

Good catch. There's already a comment to that effect in the other pattern. Centralized them to the pass-level comment.

124

As discussed offline, the "fragile interaction" is when debugging miscompiles or other debugging scenarios where one wants to reduce the number of transformations applied to see which one is at fault. Agreed that in a typical bug-free flow canonicalization should always be ok to apply indiscriminately.

Are you ok moving forward with this for now?

silvas marked an inline comment as not done.Sep 22 2020, 11:10 AM
silvas updated this revision to Diff 294132.Sep 24 2020, 12:19 PM

Remove shape.assuming canonicalization.

Per offline discussion with Stephan, it LGTM once I remove that.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 24 2020, 12:25 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.