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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Please check the comments around the code you are modifying/moving, some of them no longer describe what the code does after your changes.



================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:35
 // Add operations generating block/thread ids and grid/block dimensions at the
 // beginning of the `body` region and replace uses of the respective function
 // arguments.
----------------
Please update the comment to describe the new API


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:77
+    Operation *operandOp = operand.getDefiningOp();
+    if (!operandOp || !isInliningBeneficiary(operandOp))
+      continue;
----------------
mehdi_amini wrote:
> 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.
> This whole sinking transformation does not seem safe in general: this should check *legality* rather than "benefit".

The function just seems misnamed, should be something like `shouldSink` because it mixes validity and benefit. In practice, it only returns `true` for `constant` and `dim` operations that don't have side effects.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:104
+
+  // Remove the arguments corresponding to launch configuration and replace its
+  // uses with launch configuration operations (like gpu.block_dim, etc.).
----------------
This no longer removes the arguments, but rather updates the map.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:116
+  for (auto *inlinedOp : inlinedOperations)
+    entryBlock.push_back(inlinedOp->clone(map));
 
----------------
Will this update the users of the inlinedOps? I don't see the map updated anywhere.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp:119
+  // Clone the rest of the region.
+  for (Block &b : launchOp.body()) {
+    Block *newBlock = map.lookupOrNull(&b);
----------------
Will this work for blocks whose dominance relation is inverse of their textual order?  E.g. 

```
^entry:
  br ^bb2:
^bb1:
  "use"(%0) : (index) -> ()
  return
^bb2:
  %0 = "def"() : () -> (index)
  br ^bb1
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75287