This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a shape op that always returns a successful witness
ClosedPublic

Authored by tpopp on May 20 2020, 9:09 AM.

Details

Summary

This will later be used during canonicalization steps to replace
statically known passing constraints. This might later be removed once
witnesses can be represented as Attributes.

Diff Detail

Event Timeline

tpopp created this revision.May 20 2020, 9:09 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
tpopp updated this revision to Diff 265278.May 20 2020, 9:25 AM

Remove named TODO.

silvas accepted this revision.May 20 2020, 11:59 AM

LGTM, thanks :)

See my comment in the other patch about using this in ShapeDialect::materializeConstant

(I'm on the fence about having a const_witness {true,false} vs true_witness/false_witness ops; that latter is a bit easier to match with isa<TrueWitnessOp> so I tend to lean towards it, but the former is more consistent with const_shape/const_size and maybe a simple helper can recuperate the advantages of isa<TrueWitnessOp>'s simplicity; feel free to choose whatever makes sense)

jpienaar accepted this revision.May 20 2020, 5:25 PM
jpienaar marked an inline comment as done.
jpienaar added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
502

I'd be +1 this for consistency. If we have a witness attribute then I think we could just use std.const too.

(then little helper function to check isTrueWitness(Operation* op), which we could add already)

506

Summaries are single line sentence fragments that don't end with punctuation (https://mlir.llvm.org/docs/OpDefinitions/#operation-documentation). I see we are a bit inconsistent in this file on that already.

Might just submit a quick NFC change resolving that,

508

Should we perhaps start with some lead up before the example? And put the MLIR in a code block? (I added syntax highlighting especially for this :-))

This operation produces a constant true witness value. This often arises during constant folding/canonicalization, e.g.,

mlir
  ...

Slightly repetitive with the summary, but given custom asm syntax these will be split in the generated docs.

This revision is now accepted and ready to land.May 20 2020, 5:25 PM
herhut accepted this revision.May 25 2020, 4:20 AM

Thanks!

mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
502

I like the isTrueWitness to hide how this is implemented. Then we can move this to ConstantLike later without changing users.

tpopp updated this revision to Diff 268475.Jun 4 2020, 8:08 AM
tpopp marked 2 inline comments as done.

Instead of true_witness, have const_witness that can be true or false.

Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 8:08 AM
rriddle marked an inline comment as done.Jun 4 2020, 10:30 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
526

nit: Remove the indent inside of this block.

mlir/lib/Dialect/Shape/IR/Shape.cpp
45 ↗(On Diff #268475)

nit: Remove trivial braces.

This revision was automatically updated to reflect the committed changes.