[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:77
+    Operation *operandOp = operand.getDefiningOp();
+    if (!operandOp || !isInliningBeneficiary(operandOp))
+      continue;
----------------
herhut wrote:
> ftynse wrote:
> > 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.
> > This whole sinking transformation does not seem safe in general: this should check *legality* rather than "benefit".
> 
> Well, it should check both. You do not want to move all legal operation either :)
> 
> > 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 has purely historical reasons. Not long ago, the `gpu.launch` was closed from above, so this transformation was done when moving to function form. I have a separate pass for this in a local client, which I can send out next week. It just needs tests.
> 
> It was implemented as a "post transformation" to the outlining and I would prefer if we do not mix it into the outlining transformation itself. When written separately, the transformations are trivial.
> It was implemented as a "post transformation" to the outlining and

Pre-outlining seems easier to manage because region vs inter-procedural (and also can be kept a function pass). 

> I would prefer if we do not mix it into the outlining transformation itself. When written separately, the transformations are trivial.

Seems like we're in agreement :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75287