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

[Stephan Herhut via Phabricator via llvm-commits]

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

Thanks for cleaning this up. With comments addresses, I'd be happy to see this land.



================
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,
----------------
This is not illegal.


================
Comment at: mlir/include/mlir/Dialect/GPU/ParallelLoopMapperAttr.td:40
+//         id based on an upper bound of the number of iterations.
+def ParallelLoopDimMapperAttr :
+    StructAttr<"ParallelLoopDimMapper", GPU_Dialect,
----------------
`ParallelLoopDimMappingAttr` as it describes the mapping and not the mapper.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp:51
+        static_cast<gpu::Processor>(dimAttr.processor().getInt());
+    if (processor != gpu::Processor::SEQUENTIAL &&
+        specifiedMappings.count(processor))
----------------
Why is this invalid? You could map to the same processor but use `ceilDiv` and `modulo` in the `map` attribute to decompose the bound again.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp:83
 /// sequential after.
-static int64_t getHardwareIdForMapping(MappingLevel level, int dimension) {
+/// TODO(ravishankarm/herhut) : Make this use x for the inner-most loop that is
+/// distributed to map to x, the next innermost to y and the next innermost to
----------------
I do not understand this comment.


================
Comment at: mlir/test/Conversion/LoopsToGPU/parallel_loop.mlir:312
   %four = constant 4 : index
-  // expected-error at +2 {{cannot redefine the bound for processor 1}}
+  // expected-error at +2 {{cannot redefine the bound for processor BLOCKY}}
   // expected-error at +1 {{failed to legalize operation 'loop.parallel'}}
----------------
Now the error message no longer corresponds to what is encoded in the attribute. If I search for `BLOCKY` in the input I would not find it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76165