[PATCH] D76246: [mlir][spirv] Consolidate std.constant to spv.constant conversions

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked 4 inline comments as done.
antiagainst added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:53
+  // want to change the bitwidth. Emit a message at least.
+  if (srcAttr.getValue().isSignedIntN(dstType.getWidth())) {
+    auto dstAttr = builder.getIntegerAttr(dstType, srcAttr.getInt());
----------------
mravishankar wrote:
> +1 for this is dangerous!
Sadly although we say index cannot be negative, but AFAICT, we are actually relying on negative index values in passes like https://github.com/llvm/llvm-project/blob/bd0ca2627cfa1acde2a272347ed55d88a9751869/mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp#L126. That is the reason of this statement. 


================
Comment at: mlir/test/Conversion/StandardToSPIRV/std-to-spirv.mlir:308
+func @constant_16bit() {
+  // CHECK: spv.constant 4 : i16
+  %0 = constant 4 : i16
----------------
mravishankar wrote:
> How come these didnt become i32. I thought there is a vulkan extension needed for that.
It's governed by the `Int16` capability, which is declared for this test block. i16 used for computation does not require extra extensions (well, if used in GLSL extended instructions it [needs](http://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/master/extensions/AMD/SPV_AMD_gpu_shader_int16.html). If i16 is used in interface storage classes like StorageBuffer and others then we need extra extension, SPV_KHR_16bit_storage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76246