[PATCH] D80179: [mlir] Mark witness related Shape dialect ops as NoSideEffect.

[Theodore Popp via Phabricator via llvm-commits]

tpopp marked 3 inline comments as done.
tpopp added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:428
 
-def Shape_CstrBroadcastableOp : Shape_Op<"cstr_broadcastable", []> {
+def Shape_CstrBroadcastableOp : Shape_Op<"cstr_broadcastable", [NoSideEffect]> {
   let summary = "Determines if 2 shapes can be successfully broadcasted.";
----------------
herhut wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80179/new/

https://reviews.llvm.org/D80179