[PATCH] D73893: [MLIR][GPU] Implement initial mapping from loop.parallel to gpu.launch.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:503
+
+std::tuple<unsigned, AffineMap, AffineMap>
+extractMapAndOperand(Attribute attribute) {
----------------
I would suggest creating a struct with named fields here for better readability. And document the function plz


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:505
+extractMapAndOperand(Attribute attribute) {
+  DictionaryAttr dict = attribute.dyn_cast<DictionaryAttr>();
+  unsigned processor =
----------------
.cast here and below, otherwise you may be accessing a null pointer


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:507
+  unsigned processor =
+      dict.get("processor").dyn_cast<IntegerAttr>().getValue().getSExtValue();
+  AffineMap map = dict.get("map").dyn_cast<AffineMapAttr>().getValue();
----------------
Nit: can we factor out these names into constants?


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:517
+LogicalResult processParallelLoop(ParallelOp parallelOp, gpu::LaunchOp launchOp,
+                                  BlockAndValueMapping &cloning_map,
+                                  SmallVectorImpl<Operation *> &worklist,
----------------
Nit: mlir uses camelBack names


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:521
+  // TODO(herhut): Verify that this is a valid GPU mapping.
+  // processor ids: 0-2 block [x/y/z], 3-5 -> thread [x/y/z], 6-> sequential
+  ArrayAttr mapping = parallelOp.getAttrOfType<ArrayAttr>("mapping");
----------------
Should we use string attributes instead? E.g. having `mapping = ["thread-0", "thread-1", "thread-2", "block-0", "block-1", "block-2", "seq"]` ?


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:538
+      return val;
+    if (ConstantOp constOp = dyn_cast<ConstantOp>(val.getDefiningOp()))
+      return rewriter.create<ConstantOp>(constOp.getLoc(), constOp.getValue());
----------------
`dyn_cast_or_null` since `val` is not guaranteed to have a defining op


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:582
+            !isa<ConstantOp>(upperBound.getDefiningOp())) {
+          // TODO(herhut): Is there a helper in affine for this?
+          if (AffineMinOp minOp =
----------------
We should have patterns to fold constants into affine maps (in canonicalization), eg

%0 = constant 42 : index
%1 = afifne.min affine_map<(d0,d1)->(d0,d1)>(%0, %arg0)

should fold into

%0 = affine.min affine_map<(d0)->(d0,42)>(%0)

LMK if it's not the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73893