Page MenuHomePhabricator

[mlir] Add canonicalization for Cstr and Assuming Shape Ops.
Needs ReviewPublic

Authored by tpopp on Tue, May 19, 1:10 AM.

Details

Summary

As a first step, represent statically known passing constraint as a
true op and remove assuming ops that rely on only true_witness ops.

Diff Detail

Event Timeline

tpopp created this revision.Tue, May 19, 1:10 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
herhut added inline comments.Tue, May 19, 1:43 AM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
483

No TODO with names please.

mlir/lib/Dialect/Shape/IR/Shape.cpp
141

Why this change?

194

Why initPosition?

mlir/test/Dialect/Shape/canonicalize.mlir
91

Some tests where folding does not happen would also be great.

silvas added inline comments.Tue, May 19, 8:04 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
106

Use a fold.

186

We might not want this pattern always. For example, if we have a pass that does code motion on islands, this canonicalization can prematurely remove the island which will make that pass unable to operate properly. Let's hold off on adding it until we have a compelling use case.

325

This will segfault if getDefiningOp returns null.

mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td
19

I don't think that TableGen is buying us much here. I prefer loose C++ patterns since they are easier to debug and understand.

mlir/test/Dialect/Shape/canonicalize.mlir
119

Can you make this test have less op surface area? Maybe use an unknown op to capture the witness (or return the witness) instead of having the shape.assuming?

mehdi_amini added inline comments.Tue, May 19, 8:39 PM
mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td
19

Easier to understand is arguable: this can be a matter of getting used to it, easier to debug is an infra issue which should be fixed if any.

C++ pattern aren't ideal because we can't manipulate them statically: even if DRR is far from perfect, at least it is declarative.
In particular with the interpreted rewrite engine coming, looking into authoring such transformation in a non-C++ way is valuable (whether we use DRR or we come up with another authoring language to express these)

tpopp marked 14 inline comments as done.Wed, May 20, 9:14 AM

I split this commit up into smaller pieces.

mlir/lib/Dialect/Shape/IR/Shape.cpp
141

Accidental

186

I don't find this a compelling argument. Not all code will be in islands and these islands are for the purpose of making code conditional on some assertion. If it's statically known that the assertion is not a problem and the island cannot be removed, then the island is being misused for another purpose and blocking possible compiler optimizations.

194

Changed to opPosition as the rewriter starts where the op is.

mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td
19

I'm going to leave as tablegen. I'm not a big fan of almost all tablegen patterns for LLVM but am going along with what the infrastructure implies it wants.

mlir/test/Dialect/Shape/canonicalize.mlir
119

I split these into one for each canonicalization.

silvas resigned from this revision.Wed, May 20, 7:00 PM

Looks like this has been shraded into a couple other patches. Resigning from this master one as I reviewed the others.

mlir/lib/Dialect/Shape/IR/Shape.cpp
186

Good point!