This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Mark witness related Shape dialect ops as NoSideEffect.
ClosedPublic

Authored by tpopp on May 18 2020, 9:28 PM.

Diff Detail

Event Timeline

tpopp created this revision.May 18 2020, 9:28 PM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
herhut added inline comments.May 19 2020, 2:08 AM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
446

Marking these as NoSideEffect is dangerous. If their witness value goes unused, they will just disappear as the are considered dead at that point. That is likely not what we want.

465

Same here.

tpopp marked 3 inline comments as done.May 20 2020, 1:26 AM
tpopp added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
446

I'm removing these, but just for completeness for anyone reading this want to say that I think these being removed once unused is a good thing because I don't think these should be used as pure assertions and only to prevent incorrect code from being executed, so once the code is known to be correct, this form of assertion should be removed even if it might fire.

The problem with NoSideEffect in my opinion is the fact that they could then be hoisted out of some If-block and then turned into an assertion that would never have executed in the original location.

This revision was not accepted when it landed; it landed in state Needs Review.May 20 2020, 1:35 AM
This revision was automatically updated to reflect the committed changes.
herhut added inline comments.May 20 2020, 2:21 AM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
446

I think this is another use case for the NoSpeculation trait. Or rather, we should have an AllowsSpeculation trait to attach to operations that can be hoisted across control flow.