[PATCH] D80305: [mlir] Add folding for shape.any

[Theodore Popp via Phabricator via llvm-commits]

tpopp added inline comments.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:106
+// determined through mixtures of the known dimensions of the inputs.
+OpFoldResult AnyOp::fold(ArrayRef<Attribute> operands) {
+  // Only the last operand is checked because AnyOp is commutative.
----------------
jpienaar wrote:
> So the folder only focuses on case where the output can be fully statically computed? [this overlaps with Sean's question below]
Yes, I started to add that as a comment, but then realized that's essentially the definition of folding, so I think it would be redundant to state.


================
Comment at: mlir/test/Dialect/Shape/canonicalize.mlir:154
+// -----
+// any is not yet replaced without a constant input.
+// CHECK-LABEL: func @f
----------------
herhut wrote:
> silvas wrote:
> > Can you expand a bit on this "yet"? Is that a direction we want to go? It seems like it would be a very small change to this patch to support that, suggesting there is a deeper reason why we want to avoid that, which would be good to document here or in AnyOp::fold
> We would need to inspect operands and their types for this to work, which is more complex than just looking at constant operands. So it makes sense to land this first, if only to support static shape workloads, and expand later. A comment like `Folding of any with partially constant operands is not yet implemented` should convey this.
There's a not a specific reason to not support it. I would rather wait until there's a clear use case though to implement more of this as it's more risky given the inherent lack of safety in the operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80305