[PATCH] D74270: [mlir][GPUToSPIRV] Modify the lowering of gpu.block_dim to be consistent with Vulkan SPEC

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:71
 
+/// Pattern lowering gpu::BlockDimOp. This is separate cause the workgroup size
+/// is supposed to be constant for Vulkan. Currently the buitlin variable that
----------------
s/cause/because/


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:72
+/// Pattern lowering gpu::BlockDimOp. This is separate cause the workgroup size
+/// is supposed to be constant for Vulkan. Currently the buitlin variable that
+/// would be generated would not have a constant initializer (unsupported by
----------------
This explanation is a bit confusing to me. SPIR-V spec allows variables to have initializers; it's the Vulkan spec requires `WorkgroupSize` to be decorated onto constants. What about using the following:

... This is separate because in Vulkan workgroup size is exposed to shaders via a constant with `WorkgroupSize` decoration. So here we cannot generate a builtin variable; instead the infromation ... 


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:257
 
-template <typename SourceOp, spirv::BuiltIn builtin>
-PatternMatchResult LaunchConfigConversion<SourceOp, builtin>::matchAndRewrite(
-    SourceOp op, ArrayRef<Value> operands,
-    ConversionPatternRewriter &rewriter) const {
-  auto dimAttr =
-      op.getOperation()->template getAttrOfType<StringAttr>("dimension");
+static Optional<int32_t> getIndexForLaunchConfigOp(Operation *op) {
+  auto dimAttr = op->getAttrOfType<StringAttr>("dimension");
----------------
Nit: getLaunchConfigIndex ? This actually does not check what the op is; it's just querying the launch configuration index.


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:438
   patterns.insert<
       ForOpConversion, GPUReturnOpConversion, IfOpConversion,
+      GPUModuleConversion, GPUReturnOpConversion, ForOpConversion,
----------------
Sort these alphabetically


================
Comment at: mlir/test/Conversion/GPUToSPIRV/builtins.mlir:83
       attributes {gpu.kernel} {
-      // CHECK: [[ADDRESS:%.*]] = spv._address_of [[WORKGROUPSIZE]]
-      // CHECK-NEXT: [[VEC:%.*]] = spv.Load "Input" [[ADDRESS]]
-      // CHECK-NEXT: {{%.*}} = spv.CompositeExtract [[VEC]]{{\[}}0 : i32{{\]}}
+      // CHECK: spv.constant 32 : i32
       %0 = "gpu.block_dim"() {dimension = "x"} : () -> index
----------------
Let's add some notes here to explain where this magic number comes from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74270