[PATCH] D75287: [mlir][GPU] Expose the functionality to create a gpu.GPUFuncOp from a gpu.GPULaunchOp

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
Comment at: mlir/include/mlir/Dialect/GPU/Passes.h:21
 
+class LogicalResult;
 class MLIRContext;
----------------
ftynse wrote:
> `struct`, otherwise you'll break windows builds
THanks! Interesting that the build bot passed.


================
Comment at: mlir/include/mlir/Dialect/GPU/Passes.h:53
+/// region.
+LogicalResult sinkInstructionsIntoLaunchOp(gpu::LaunchOp launchOp);
+
----------------
ftynse wrote:
> Bikeshed nit: `sinkOperationsIntoLaunchOp`. "Intructions" aren't a thing in MLIR.
Good point. I have conflated the two mentally cause of that.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:112
+    }
+    if (startSize == processed.size()) {
+      return launchOp.emitError(
----------------
ftynse wrote:
> Can this happen in a valid IR? If not, I would rather assert. Otherwise, please drop trivial braces
I think so, but I am not sure. Will leave it as an error, and remove braces


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:236
+          return WalkResult::interrupt();
+        }
+        gpu::GPUFuncOp outlinedFunc =
----------------
mehdi_amini wrote:
> Note: this is still not decoupled from this pass right now (i.e. not tested in isolation, etc.): we still have "outlining" and "sinking" part of the same pass, can't they be separated?
They are separate functions. I have no visibility into the clients of the pass. So if any user of the pass is relying on sinking happening then removing the sinking would "potentially" break. One could argue that then it is incorrect usage since the gpu.launch_func op gets updated accordingly, but at this point I would rather keep this change as an NFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75287