Page MenuHomePhabricator

[MLIR] Add shape.witness type and ops
ClosedPublic

Authored by tpopp on Mon, May 11, 7:30 AM.

Details

Summary

These represent shape based preconditions on execution of code.

Diff Detail

Event Timeline

tpopp created this revision.Mon, May 11, 7:30 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
tpopp added a comment.EditedMon, May 11, 7:49 AM

Please don't review yet. This is a work in progress including the op definitions.

tpopp updated this revision to Diff 263414.Tue, May 12, 6:24 AM

Update descriptions of operations.

silvas accepted this revision.Tue, May 12, 1:42 PM
silvas added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
24

typo "using relying"

25

"They are only used as a structural device in the compiler to maintain ordering."

32

typo Cod

399

def name inconsistent with op name.

436

def name is inconsistent with op name.

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

nit: newline after block comment

mlir/test/Dialect/Shape/ops.mlir
71

Let's move the shape.any into the region?

Great start. Good to go with comments addressed.

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

Please keep these sorted.

mlir/test/Dialect/Shape/ops.mlir
73

This needs updating to the new name for the region. Looking at the example, assuming_region conveys the intent of the op much better. We could even drop the region so that it reads (with some potential later custom assembly form)

%res = shape.assuming %w3 do {
} : !shape.shape
tpopp updated this revision to Diff 264185.Fri, May 15, 2:08 AM
tpopp marked 8 inline comments as done.

Update op descriptions and test based on feedback.

tpopp marked an inline comment as done.Fri, May 15, 2:18 AM
tpopp added inline comments.
mlir/test/Dialect/Shape/ops.mlir
73

Done. assuming_all is a bad name I think then (I would expect it to be a multiple input form of assuming). I will revisit that later though.

tpopp marked an inline comment as done.Fri, May 15, 2:19 AM
This revision was not accepted when it landed; it landed in state Needs Review.Fri, May 15, 5:53 AM
Closed by commit rGa26883e5aa14: [MLIR] Add shape.witness type and ops (authored by Tres Popp <tpopp@google.com>). · Explain Why
This revision was automatically updated to reflect the committed changes.
herhut added inline comments.Fri, May 15, 6:18 AM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
346

LLVM/MLIR code does not have user name based TODO's typically.

mehdi_amini added inline comments.Fri, May 15, 8:20 PM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
451

Nit: your doc shows nice example where the syntax isn't generic, could you use the declarative assembly syntax for all these ops?