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

[Stephan Herhut via Phabricator via llvm-commits]

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


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:576
+/// To note the end of the current scope in case a loop or conditional was
+/// inserted, a sentinel (the `gpu.launch` operation) is inserted into the
+/// worklist. This signals the processor of the worklist to pop the rewriter
----------------
ftynse wrote:
> Nit: why not `nullptr` as a sentinel?
I dislike nullptr as sentinel because it too easily sneaks in and then produces hard to debug errors. The `gpu.launch` on the other hand definitely won't. I would have created an explicit sentinel but that seemed a bit overkill.


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:748
+    // TODO: This is only correct if there either is no further loop.parallel
+    //       nested or this code is side-effect free. Otherwise we might need
+    //       predication.
----------------
ftynse wrote:
> Can we just walk recursively and check for the no side effects trait, returning matchFailure if there are side effects?
I added some testing code that is conservative but ensures correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73893