[Mlir-commits] [mlir] [mlir][gpu] Introduce `gpu.dynamic_shared_memory` Op (PR #71546)

[Oleksandr Alex Zinenko]

================
@@ -554,6 +555,95 @@ static IntegerAttr wrapNumericMemorySpace(MLIRContext *ctx, unsigned space) {
   return IntegerAttr::get(IntegerType::get(ctx, 64), space);
 }
 
+/// Generates a symbol with 0-sized array type for dynamic shared memory usage,
+/// or uses existing symbol.
+LLVM::GlobalOp
+getDynamicSharedMemorySymbol(ConversionPatternRewriter &rewriter,
+                             gpu::DynamicSharedMemoryOp op,
+                             const LLVMTypeConverter *typeConverter,
+                             MemRefType memrefType, unsigned alignmentBit) {
+  std::optional<LLVM::GlobalOp> existingGlobalOp;
+
+  LLVM::LLVMFuncOp funcOp = op->getParentOfType<LLVM::LLVMFuncOp>();
+  assert(funcOp && "cannot find llvm.func op");
+
+  gpu::GPUModuleOp moduleOp = funcOp->getParentOfType<gpu::GPUModuleOp>();
+  assert(moduleOp && "cannot find gpu.module op");
+
+  // Use already generated global op if it exists
+  int index = 0;
+  std::string prefix = llvm::formatv("__shmem_{0}", funcOp.getSymName());
+  moduleOp->walk([&](LLVM::GlobalOp globalOp) {
+    if (auto arrayType = dyn_cast<LLVM::LLVMArrayType>(globalOp.getType())) {
+      if (arrayType.getNumElements() == 0) {
+        existingGlobalOp = globalOp;
+        return WalkResult::interrupt();
+      }
+    }
+    if (globalOp.getSymName().startswith(prefix))
+      index++;
----------------
ftynse wrote:

> I believe this is due to the dialect conversion framework only erasing converted operations after all rewrites have been performed.

Correct, thanks for investigation.

I agree that we are left with no choice but to do manual renaming. My suggestion is to (1) collect all names of globals upfront in the pattern before the and store them in StringSet<> (rather than walking over the IR in a loop) and (2) factor out [this code](https://github.com/llvm/llvm-project/blob/d62f040418bd167d1ddd2b79c640a90c0c2ea353/mlir/lib/IR/SymbolTable.cpp#L211-L216) into a utility function, possibly function template, and call it from here to ensure the renaming scheme is consistent (that implementation also avoids constant string reallocation).

I don't expect that there may be another pattern interfering with this one and somehow adding the globals that would invalidate the collected set. It would require that pattern to run in a separate thread and do concurrent mutation of the common parent, which shouldn't be allowed anyway. (This raises the question about such "non-local" patterns that cannot be correctly applied in passes rooted lower than the topmost op the pattern may modify, module in this case, without no indication anywhere but the pattern code. This is not specific this PR, so not blocking.)

https://github.com/llvm/llvm-project/pull/71546