[PATCH] D72129: [mlir] Add in-dialect lowering of gpu.all_reduce.

[Christian Sigg via Phabricator via llvm-commits]

csigg added a comment.

> The approach you took for building code is very similar to our motivation for declarative builders (aka EDSC), maybe it's time to reconsider those again.

Yes, I'm happy to change this to EDSC as a follow-up. For now I think it is easier to keep it similar to the existing lowering to NVVM.



================
Comment at: mlir/lib/Dialect/GPU/Transforms/AllReduceLowering.cpp:150
+    auto bufferType =
+        MemRefType::get({kWarpSize}, valueType, ArrayRef<AffineMap>{}, 3);
+    return funcOp.addWorkgroupAttribution(bufferType);
----------------
ftynse wrote:
> csigg wrote:
> > ftynse wrote:
> > > Nit: let's have a named constant for the address space 
> > Punted by just adding a local variable.
> I'll add that later anyway :)
Thanks!


================
Comment at: mlir/lib/Dialect/GPU/Transforms/AllReduceLowering.cpp:354
+    };
+    while (funcOp.walk(callback).wasInterrupted()) {
+    }
----------------
ftynse wrote:
> Could you please add a comment on why this approach is necessary?  I understand that you need to operate on the GPUFuncOp level because you modify the GPUFuncOp itself, which would be incorrect from a nested operation (AllReduceOp). I don't understand why do you need to interrupt the walk after each rewrite. Is it because of some state invalidation?
Correct, the walk iterators get invalidated from the replace. Comment added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72129