[PATCH] D72223: [LLVM] [MLIR] Introduce affine graybox op

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/docs/Dialects/Affine.md:612
+be a *memref*.  The operands bind 1:1 to its region's arguments.  The op can't
+use any memrefs defined outside of it, but can use any other SSA values that
+dominate it. Its region's blocks can have terminators the same way as current
----------------
bondhugula wrote:
> mehdi_amini wrote:
> > I don’t think you addressed my concerns on this topic?
> I think I responded to everything and included all of the arguments in the RFC:
> https://github.com/bondhugula/llvm-project/blob/graybox/mlir/rfc/rfc-graybox.md
> Could you just provide a summary list of the concerns you still have - either here or on that thread as you prefer?
I explained my concerns in the original thread https://groups.google.com/a/tensorflow.org/d/msg/mlir/O5PXVbtlSng/3SXmxDiLAAAJ 

Here is what I wrote:

> I am trying to not consider affine at all here. I wrote these example to try to illustrate how MLIR region/op interaction are structured opaquely to be able to derive cross-dialect invariants in general.
> The invariant I am presenting above is independent from any dialect, let me abstract the type further:
> 
> func @foo(%value : !dialect.type) {
> op.with_region {
> any.op(%value) : (!dialect.type) -> ()
> }
> }
> 
> If I look at it generically, here is my take on it:
> a) The `op.with_region` defines the semantic of its immediate region, so it can either accept or reject `any.op`.
> b) Let's assume that `op.with_region` does not know anything about `any.op` (no traits, no prior knowledge, the `any.op` could be unregistered at this point).
> c) For this IR to be valid, `op.with_region` must be accepting unknown op (like `any.op`).
> d) From the perspective of `op.with_region`, the `any.op` is like an opaque call to some code it cannot see.
> 
> But what if `any.op` has a region?
> 
> func @foo(%value : !dialect.type) {
> op.with_region {
> any.op(%value) ({
> ^bb(%value_inside):
> // do something with %value_inside (explicitly captured)
> }) : (!dialect.type) -> ()
> }}
> 
> Here what changes is that:
> e) any.op has a region now. Unless `op.with_region` forbids unknown operation from having a region in its verifier, and it does not have specific handling for `any.op`, then the IR should be valid.
> f) Since %value_inside is explicitly captured, without knowing specifically `any.op`, then the uses of %value_inside cannot be restricted by `op.with_region` but only by `any_op`.
> g) For any practical purpose here, there should not be any difference between this form and the first one above.
> 
> Finally, what if `any.op` has implicit capture?
> 
> func @foo(%value : !dialect.type) {
> op.with_region {
> any.op() ({
> // do something with %value (implicit capture instead of explicit)
> }) : (!dialect.type) -> ()
> }}
> 
> Now:
> h) `any.op()` is implicitly capturing %value.
> g) Without more information about `any.op` (traits, etc.), this should be equivalent to the explicit capture case: if the IR was valid the first and second case, then it should be valid here.
> 
> If we don't have these properties, and if `op.with_region` can constrain the validity of the region attached to `any.op`, then `any.op` is not longer in control of the semantics of the enclosed region. No transformation can operate on `any.op` without knowing all of the enclosing operations, since these can add arbitrary restrictions.
> For example, this is a valid IR (you can pipe this in mlir-opt right now):
> 
> module {
> "d1.op1" () ({
> "d2.op2" () ({
> module {
> func @bar() {
> return
> }
> func @foo() {
> call @bar() : () -> ()
> return
> }
> }
> "d2.done" () : () -> ()
> }) : () -> ()
> }) : () -> ()
> }
> 
> If I get the inner @foo function, and would like to inline the call to @bar, what do I need to check to ensure I can?
> If the FuncOp defines the semantic of the region, then the FuncOp should control itself whether it allows to inline or not, and I should query FuncOp for @foo, CallOp for the call-site, FuncOp for @bar, and likely the op inside @bar that I am about to inline.
> 
> If you allow to put restriction on what can happen inside @foo(), based on the enclosing operation, then you can't inline unless you ensure that all the enclosing operation will be happy with it (so you need to check the enclosing modules, but also "d1.op1" and "d2.op2").
> 
> Basically, this would be breaking the composability of the IR: you couldn't assemble independent pieces and reason about them independently. I don't know why we would want that, here we really want to reason about the functions in the inner module independently if they are surrounded by "d1.op1"() and "d2.op2"() like here (otherwise none of the current passes in MLIR are correct).
> 

I don't think you answered these points that explain why I am not convinced it is OK to have explicit capture just for memref. 
I don't think it is necessary to have explicit capture of memref either by the way, dropping this may help getting forward right now.

You answered the email above in the thread, but you didn't address it I believe, you wrote "I'll respond to the connection to the affine.graybox proposal in another post" but I didn't see another post after that.

(I'm on vacation till 1/10, expect some delays in answers)


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

https://reviews.llvm.org/D72223