[PATCH] D73944: [mlir][wip] Start Shape dialect

[Stephan Herhut via Phabricator via llvm-commits]

herhut accepted this revision.
herhut added a comment.

Thanks for starting this. I'd be happy to see this land.



================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:110
+
+// TODO: consider just making a constant op instead.
+def Shape_ConstantDimOp : Shape_Op<"constant_dim", []> {
----------------
jpienaar wrote:
> herhut wrote:
> > +1 to the constant op. Also, why expose a -1 here? Why not use
> > 
> > shape.constant unknown : !shape.size
> > shape.constant invalid : !shape.size
> > shape.constant <positive integer> : !shape.size
> > 
> > and then parse that internally into some special integer values? This would leave more flexibility to change the backing type to something other than int later.
> Mostly as it made writing tests easier while allowing reusing the integer attribute parser/dense storage and so switch later. But I can start with an ArrayAttr and then do the above.
> 
> For invalid case I feel constant doesn't highlight the error state well enough and would rather that be a separate op that takes a string attribute. WDYT?
I like the idea. Invalid shapes should always have some reason attached to them to ease debugging whereas normal constants probably don't. Something like `shape.error`.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:190
+
+           output[i] = lhs[i] if lhs[i] == rhs[i] or rhs[i] is unknown/undefined
+                     = rhs[i] if lhs[i] is unknown/undefined
----------------
jpienaar wrote:
> stellaraccident wrote:
> > nicolasvasilache wrote:
> > > nicolasvasilache wrote:
> > > > This is frontend logic that I do not think should leak into MLIR.
> > > I meant into MLIR *core*, sorry.
> > I tend to agree. We do need this to lower frontend dialects that choose to implement numpy-style broadcasting, but that does not mean it needs to be in core (or at least in this level of core -- I could see having a numpy_broadcast_shape op somewhere).
> > 
> > I suspect that there may have been some thought of needing this in some follow-on analysis algorithms. It would probably be better to discuss more in that context.
> Correct, we need this for frontend dialects (TF, TFLite, XLA [client] HLO, etc) to be able to express their shape constraints. it need not be used by standard etc. and we could add specialization that only works for ranked types. But I see unranked inputs and broadcast behavior as common for many frontend dialects and this similar to broadcastable trait or signed integers. We can move the op into a different file.
> 
> Now, of course nobody is forced to use this and standard ops will never generate nor need to solve shape equations using this. 
Ultimately, we could think of making the shape dialect and the machinery built on top of it extensible so that other front-ends can add their own high-level shape operations to model their constraints. In such a world, I would agree with @nicolasvasilache that the numpy-style broadcast operation should live with the corresponding front-end dialect.
However, as a first step, we should build a closed system that is powerful enough to model constraints for the input languages we care about (which includes HLO). This requires computing shapes for broadcast, so we should have the operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73944