[PATCH] D74954: Add a basic tiling pass for parallel loops

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Just adding some minor nit comments!

>> We already several implementations of loop tiling (lib/Transforms/LoopTiling.cpp, lib/Dialect/Linalg/Transforms/Tiling.cpp, lib/Transforms/Utils/LoopUtils.cpp). Have you considered generalizing them instead of introducing yet another one? I suppose parallel loop nest as an operation removes the need for any supplementary preconditions, but the mechanics of the transformation should be very similar between parallel and sequential loops.



> The complexity of the tiling passes is in the dependency analysis and not the rewriting itself. This one is purely structural. We would need to split the actual tiling rewrite form the analysis with interfaces for generating the tiled operation. I am not sure that is worth the code we would actually get to reuse.

I think it might be worth it since affine parallel for would be another candidate for tiling.



================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:40
+  SmallVector<Value, 2> tileSizeConstants;
+  for (size_t i = 0; i != op.upperBound().size(); ++i) {
+    if (i < tileSizes.size()) {
----------------
`size_t i = 0; i != op.upperBound().size();` -> `size_t i = 0, end = op.upperBound().size(); i != end;` ?


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:115
+      getInnermostNestedLoops(&block, mostNestedParallelOps);
+    }
+    for (ParallelOp pLoop : mostNestedParallelOps) {
----------------
please drop {} for single line for and if-else statements 


================
Comment at: mlir/test/Dialect/Loops/parallel-loop-tiling.mlir:37
+// CHECK:           return
+
+func @tile_nested_innermost() {
----------------
Please add `// -----` between tests for split-input-file to work


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74954