Page MenuHomePhabricator

[MLIR][Standard] Add `assert` operation to the standard dialect
ClosedPublic

Authored by frgossen on Jul 3 2020, 12:51 AM.

Diff Detail

Event Timeline

frgossen created this revision.Jul 3 2020, 12:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 12:51 AM
herhut added a comment.Jul 3 2020, 2:03 AM

Can you post on discourse that you are looking to add this to std?

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
452

I would make printing the message a requirement. Instead, how about

Optionally, a runtime might use the provided message to print an error.

frgossen updated this revision to Diff 275344.Jul 3 2020, 3:23 AM
frgossen marked an inline comment as done.

Update doc

herhut accepted this revision.Jul 3 2020, 6:06 AM

Looks good to me. Maybe wait for reactions on discourse.

This revision is now accepted and ready to land.Jul 3 2020, 6:06 AM
stellaraccident accepted this revision.Jul 3 2020, 8:30 AM
stellaraccident added a subscriber: stellaraccident.

Thanks for posting on discourse.

Makes sense to me. We can lower this to our vm.cond_fail pseudo-op (https://github.com/google/iree/blob/bf894c869e281985f392775c0cc0bb199201b477/iree/compiler/Dialect/VM/IR/VMOps.td#L1875), which forget lowers to a cond_br with a vm.fail terminator in the target block.

bondhugula requested changes to this revision.Jul 3 2020, 7:47 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
451

I think the semantics would need to be spelt out. Is execution guaranteed to abort on failure? The next line only talks about the error message printing being optional.

This revision now requires changes to proceed.Jul 3 2020, 7:47 PM
bondhugula added inline comments.Jul 3 2020, 7:48 PM
mlir/test/Dialect/Standard/ops.mlir
22

This is inconsistent with the doc description which says the error message is optional.

frgossen updated this revision to Diff 275652.Jul 6 2020, 4:09 AM
frgossen marked an inline comment as done.

Update doc

frgossen marked 2 inline comments as done.Jul 6 2020, 4:10 AM
frgossen added inline comments.
mlir/test/Dialect/Standard/ops.mlir
22

The error message is required. What the runtime does with it is up to the user though.

frgossen marked an inline comment as done.Jul 6 2020, 5:21 AM
bondhugula accepted this revision.Jul 6 2020, 11:53 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
451

Nit: Boolean -> boolean

453

Nit: You can use the whole line - here and above.

This revision is now accepted and ready to land.Jul 6 2020, 11:53 AM
rriddle added inline comments.Jul 6 2020, 7:00 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
467

nit: You don't need to specify this if it is empty.

herhut accepted this revision.Jul 7 2020, 1:07 AM

Thanks

silvas added a subscriber: silvas.Mon, Jul 13, 11:25 AM
silvas added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
457

There is no "assert" function. (it's a macro; see my post on discourse). I think we should just remove this line from the description.

frgossen updated this revision to Diff 277727.Tue, Jul 14, 2:59 AM
frgossen marked 5 inline comments as done.

Update documentation

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
453

I always found line breaks after sentences more semantic, they lead to more readable diffs, and prevent line fragmentation when the text is changed.
Should I change this anyways?

This revision was automatically updated to reflect the committed changes.
jpienaar added inline comments.Tue, Jul 14, 6:03 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
453

Yes please. Keeping consistency with the rest of the file and project comment style is important.

We have been following "regular" writing practices wrt lines vs sentences vs paragraph as one is optimising for the reader and not for changes/diffing (where tools do a good job with highlighting already and changing isn't the common case). Inconsistencies/deviations make readers wonder about the significance and impairs reading. Of course if there is a discussion where we adopt a new documentation convention, then this may change.

frgossen marked 2 inline comments as done.Tue, Jul 14, 6:53 AM
frgossen added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
453

Will keep this in mind and corrected the documentation in a separate CL.
See https://reviews.llvm.org/D83769

frgossen marked an inline comment as done.Thu, Jul 16, 2:16 AM