[PATCH] D76165: [mlir][GPU] Use StructAttr to drive lowering from loop.parallel to gpu.launch

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/GPU/ParallelLoopMapper.h:52
+///   the ploopOp.
+/// - the mapping does not map multiple loops to the same processor.
+LogicalResult setMappingAttr(Operation *op,
----------------
mravishankar wrote:
> herhut wrote:
> > This is not illegal.
> I can remove that check, but I was going by what is here : https://github.com/llvm/llvm-project/blob/5c261c9c452959985de19540c168b224af24e2d3/mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp#L673
Add a TODO for Stephan to support this case in the pipeline and update the check? :)


================
Comment at: mlir/include/mlir/Dialect/GPU/ParallelLoopMapper.h:51
+/// - the number of DimMapperAttr provided is same as the number of loops of
+///   the ploopOp.
+/// - the mapping does not map multiple loops to the same processor.
----------------
Nit: ploopOp does not refer to anything in the code


================
Comment at: mlir/include/mlir/Dialect/GPU/ParallelLoopMapperAttr.td:20
+
+def BLOCKX : I64EnumAttrCase<"BLOCKX", 0>;
+def BLOCKY : I64EnumAttrCase<"BLOCKY", 1>;
----------------
antiagainst wrote:
> I'm wondering the convention here. Why all cap letters? `BlockX`, `ThreadY`/`Sequential`/etc. seems better to me.
+1 for CamelCase. I used the same in the LLVM dialect for enumerants. 


================
Comment at: mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp:49-50
+  for (auto dimAttr : mapping) {
+    gpu::Processor processor =
+        static_cast<gpu::Processor>(dimAttr.processor().getInt());
+    if (processor != gpu::Processor::SEQUENTIAL &&
----------------
You have `getProcessor` for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76165