[PATCH] D76165: [mlir][GPU] Use StructAttr to drive lowering from loop.parallel to gpu.launch

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
Comment at: mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp:526
+
+static unsigned getLaunchOpArgumentNum(gpu::Processor processor) {
+  switch (processor) {
----------------
antiagainst wrote:
> Isn't this just `static_cast<unsigned>(processor)` excluding `gpu::Processor::SEQUENTIAL`? Do we need this explicit switch here?
I am very uncomfortable relying on the enum value implicitly (especially mapping between two separate "enums" based on enum value). If one of the enums change (due to adding new enums), then it can lead to weird errors that can be hard to track down. Plus this is more readable IMO. Worth the cost of a switch-case?


================
Comment at: mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp:49-50
+  for (auto dimAttr : mapping) {
+    gpu::Processor processor =
+        static_cast<gpu::Processor>(dimAttr.processor().getInt());
+    if (processor != gpu::Processor::SEQUENTIAL &&
----------------
ftynse wrote:
> You have `getProcessor` for this
Oh yes! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76165