[PATCH] D73348: Add lowering for loop.parallel to cfg.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

Looks generally okay to me. I don't see the point of cloning before erasing, but can live with it pending a clean-up.



================
Comment at: mlir/lib/Conversion/LoopToStandard/ConvertLoopToStandard.cpp:275
+  // Now copy over the contents of the body.
+  for (auto &op : parallelOp.body().front().without_terminator()) {
+    rewriter.clone(op, mapping);
----------------
Ultra-nit: drop trivial braces.


================
Comment at: mlir/lib/Conversion/LoopToStandard/ConvertLoopToStandard.cpp:276
+  for (auto &op : parallelOp.body().front().without_terminator()) {
+    rewriter.clone(op, mapping);
+  }
----------------
Have you considered moving the region instead of cloning (`inlineRegionBefore`) and then calling `replaceAllUsesWith` to update induction variables?


================
Comment at: mlir/test/Conversion/convert-to-cfg.mlir:174
+
+func @std_parallel_loop(%arg0 : index, %arg1 : index, %arg2 : index,
+                        %arg3 : index, %arg4 : index) {
----------------
`std` looks like leftover from the temporary state when loops where in the standard dialect, let's drop it here (and later clean up the tests above)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73348