[PATCH] D79508: [mlir][gpu] Refactor functions for workgroup and private buffer attributions.

[Stephan Herhut via Phabricator via llvm-commits]

herhut added a comment.

Thanks for the cleanup.



================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:196
     /// the workgroup memory
-    ArrayRef<BlockArgument> getWorkgroupAttributions() {
-      auto begin =
-          std::next(getBody().front().args_begin(), getType().getNumInputs());
-      auto end = std::next(begin, getNumWorkgroupAttributions());
-      return {begin, end};
-    }
+    ArrayRef<BlockArgument> getWorkgroupAttributions();
 
----------------
These getters that give access to a range should remain in the header to enable inlining (and maybe avoid materializing the ArrayRef).


================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:216
 
+    /// Returns the name of the attribute containing the number of buffers
+    /// located in the private memory.
----------------
I don't think this is needed. The contract could simple be that all trailing operands after function arguments and workgroup attributions are private. Storing an extra count does not add benefit. You would then need to verify that arguments + all attributions = operand count. 

So let's just drop the extra attribute.


================
Comment at: mlir/lib/Dialect/GPU/IR/GPUDialect.cpp:478
+  auto end = std::next(begin, getNumPrivateAttributions());
+  return {begin, end};
+}
----------------
This would just be `{begin, getBody().front().args_end()}`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79508