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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:72
 
+  // Prune those that can be inlined.
+  llvm::SetVector<Value> inlinedOperands;
----------------
Please don't use "inline" for other aspect that the inliner. 

What about sink?


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:75
+  llvm::SetVector<Operation *> inlinedOperations;
+  for (auto operand : operands) {
+    Operation *operandOp = operand.getDefiningOp();
----------------
Nit: don't use auto when it does not improve the readability (line 93 below is explicit for instance)


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:77
+    Operation *operandOp = operand.getDefiningOp();
+    if (!operandOp || !isInliningBeneficiary(operandOp))
+      continue;
----------------
This whole sinking transformation does not seem safe in general: this should check *legality* rather than "benefit". 
Also it isn't clear to me why this is done during the outlining and not as a pre-pass. The launch operation with the region abstraction seems perfectly suited to model this. I rather have this exposed in a separate API / as a separate step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75287