[PATCH] D80411: [mlir][spirv] Lower allocation/deallocations of workgroup memory.

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:221
+
+    // Convert the alloc to a 1-D array of the same number of bytes. The element
+    // type might get converted to a different bitwidth type based on the target
----------------
antiagainst wrote:
> I feel we should place this logic inside the type converter itself. This is basically getting the SPIR-V "equivalent" lowered memref type for the input memref. Letting each pattern figuring out this is not the proper separation of concerns. Fine for now given this is only used by allocation; but can we put a TODO here to remind ourselves?
Yup I agree and obviously the type converter already handles this. So removing this from here. Thanks!


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:265
+      rewriter.setInsertionPointToStart(&entryBlock);
+      auto varOps = entryBlock.getOps<spirv::GlobalVariableOp>();
+      std::string varName =
----------------
antiagainst wrote:
> Do we need to unique the symbol manually here? I remember there is infrastructure support for it; but cannot recall off the top of my head.
I verified that we do. I got an error with two allocations if I didnt do this. (Adding a test for that as well).


================
Comment at: mlir/test/Conversion/GPUToSPIRV/load-store.mlir:61
       %13 = addi %arg4, %3 : index
+      // CHECK: %[[ZERO:.*]] = spv.constant 0 : i32
+      // CHECK: %[[OFFSET1_0:.*]] = spv.constant 0 : i32
----------------
antiagainst wrote:
> Might just want to add `-canonicalize` to simplify here? I know it's kinda tiding two passes together but in general canonicalization is not changing too frequently and we can have more readable tests here. But up to you. :)
True. But this will probably need to make changes to unrelated tests. Would prefer to do this in a later CL.


================
Comment at: mlir/test/Conversion/StandardToSPIRV/alloc.mlir:22
+}
+//     CHECK: spv.globalVariable @[[VAR:.+]] : !spv.ptr<!spv.struct<!spv.array<20 x f32, stride=4> [0]>, Workgroup>
+//     CHECK: func @alloc_dealloc_workgroup_mem
----------------
antiagainst wrote:
> Ah, this should not have layout annotation at all. Variables in Workgroup should not be explicitly laid out. The layout handling is problematic in the type converter. Could you turn off layout annotation for Workgroup memrefs in TypeConverter? Just do not pass in offsets when handling Workgroup in `convertMemrefType`.
THanks. Fixed!


================
Comment at: mlir/test/Conversion/StandardToSPIRV/alloc.mlir:35
+
+// TODO: Uncomment this test when the extension handling correctly
+// converts an i16 type to i32 type and handles the load/stores
----------------
antiagainst wrote:
> You mean fix? This test is not commented out at the moment.
Damn. Thanks for catching it. Commented now.
Actually looking at this test again, I realize that if this test worked as expected the generated spv.AtomicOr/spiv.AtomicAnd operations would have the wrong scope. Adjusted the code to handle that, but this cant be tested without the underlying issue being fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80411