[PATCH] D75599: [mlir] support conversion of parallel reduction loops to std

[Alex Zinenko via Phabricator via llvm-commits]

ftynse marked 3 inline comments as done.
ftynse added inline comments.


================
Comment at: mlir/lib/Conversion/LoopToStandard/ConvertLoopToStandard.cpp:284
+  auto range = parallelOp.initVals();
+  iterArgs.assign(range.begin(), range.end());
+  bool first = true;
----------------
pifon2a wrote:
> will this work? 
> ```
> SmallVector<Value, 4> iterArgs = llvm::to_vector(parallelOp.initVals());
> ```
Yes, thanks!


================
Comment at: mlir/lib/Conversion/LoopToStandard/ConvertLoopToStandard.cpp:311
+    }
+    first = false;
+
----------------
pifon2a wrote:
> nit:
> ```
> 
>     if (first) {
>       // Store the results of the outermost loop that will be used to replace
>       // the results of the parallel loop when it is fully rewritten.
>       loopResults.assign(forOp.result_begin(), forOp.result_end());
>       first = false;
>       continue;
>     } 
>     // A loop is constructed with an empty "yield" terminator by default.
>     // Replace it with another "yield" that forwards the results of the nested
>     // loop to the parent loop. We need to explicitly make sure the new
>     // terminator is the last operation in the block because further transfoms
>     // rely on this.
>     rewriter.setInsertionPointToEnd(rewriter.getInsertionBlock());
>     rewriter.replaceOpWithNewOp<YieldOp>(
>         rewriter.getInsertionBlock()->getTerminator(), forOp.getResults());
>  ```
I raised the `first = false`, but kept the `else` branch because the setInsertionPoint is necessary in both cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75599