[PATCH] D74963: [MLIR][GPU] Implement a simple greedy loop mapper.

[Stephan Herhut via Phabricator via llvm-commits]

herhut added inline comments.


================
Comment at: mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp:44
+/// sequential after.
+int64_t getHardwareIdForMapping(MappingLevel level, int dimension) {
+  if (dimension >= kNumHardwareIds || level == Sequential)
----------------
ftynse wrote:
> This looks like it would be better placed next to the code that consumes hardware ids, that is the ploop->gpu.launch transformation.
This is just an elaborate way of counting. The consumer does not use these levels.


================
Comment at: mlir/test/Dialect/GPU/mapping.mlir:56
+// CHECK-SAME:             {bound = affine_map<(d0) -> (d0)>, map = affine_map<(d0) -> (d0)>, processor = 5 : i64}, 
+// CHECK-SAME:             {bound = affine_map<(d0) -> (d0)>, map = affine_map<(d0) -> (d0)>, processor = 6 : i64}]}
+// CHECK:      {mapping = [{bound = affine_map<(d0) -> (d0)>, map = affine_map<(d0) -> (d0)>, processor = 0 : i64}, 
----------------
ftynse wrote:
> This test made me wonder about the semantic choice of saying an inner loop in a loop nest being mapped to "sequential" execution as opposed to "don't care about order" . Saying it's sequential implies the first iteration must be completed before starting the second and so on, which isn't the semantics of the original loop nest and isn't that of the generated code that does not include a synchronization after the nested parallel loop.
You should see this more as "it is mapped to a sequential loop" and describes the generated code rather than specific execution order. The overall execution order is a result of the overall mapping and constrains the original parallel execution semantics (which did not assume any order) into a specific order that assumes some order. This should always be valid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74963