[PATCH] D73465: Add gpu::LaunchOp::addKernelArgument.

[Stephan Herhut via Phabricator via llvm-commits]

herhut marked 2 inline comments as done.
herhut added inline comments.


================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:487
+    incovation of the `gpu.func`.
+    As `gpu.launch` does not return any values, it is expected that a
+    corresponding `gpu.return` also has no operands.
----------------
ftynse wrote:
> herhut wrote:
> > ftynse wrote:
> > > This says "gpu.launch" but the line above says "gpu.func". Let's use one everywhere and say the other is equivalent.
> > But they are not. When used inside of a launch, it cannot have operands. I could maybe state that gpu.launch is considered equivalent to a void function? I found it surprising that launch now has a return (as opposed to the terminator). That moves it closer to a function where it should feel more like a loop. WDYT about adding the terminator op back or is that too many operations?
> Then it's even more confusing than I thought. I'm fine with having a separate terminator for `launch`.
I will add it back in a new diff.


================
Comment at: mlir/lib/Dialect/GPU/IR/GPUDialect.cpp:283
+      return (block.back().emitError()
+              << "expected '" << gpu::ReturnOp::getOperationName() << "' or a "
+              << "terminator with successors")
----------------
rriddle wrote:
> nit: You can also use `append` to add arguments:
> 
> ```
> emitError().append(..., ..., ...).attachNote(...).append(..., ..., ...);
> ```
Thanks! I will do this in a new diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73465