This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add shape.witness type and ops
ClosedPublic

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

Details

Summary

These represent shape based preconditions on execution of code.

Diff Detail

Event Timeline

tpopp created this revision.May 11 2020, 7:30 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
tpopp added a comment.EditedMay 11 2020, 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.May 12 2020, 6:24 AM

Update descriptions of operations.

silvas accepted this revision.May 12 2020, 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.May 15 2020, 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.May 15 2020, 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.May 15 2020, 2:19 AM
This revision was not accepted when it landed; it landed in state Needs Review.May 15 2020, 5:53 AM
This revision was automatically updated to reflect the committed changes.
herhut added inline comments.May 15 2020, 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.May 15 2020, 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?

rriddle added inline comments.May 27 2020, 1:02 PM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
358

nit: newline before the code sample.

459

Can we remove this?

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

Seems like this should move to using StringSwitch at this point.