Hiden regresion on OpImageWrite

[mnaczk]

Commit https://github.com/KhronosGroup/SPIRV-LLVM-Translator/commit/b3e10f5844a432716a9214ef341499d2861e146b caused a regression on OpenCL instruction write_imagei(image_wo, (int4)(0,0,0,0), (int4)(0,0,0,0));

Without the above commit SPV-IR is call spir_func void @_Z18spirv_ImageWritePU3AS133__spirv_Imagevoid_2_0_0_0_0_0_1Dv4iS2 with the above commit SPV-IR is call spir_func void @_Z18spirv_ImageWritePU3AS133__spirv_Imagevoid_2_0_0_0_0_0_1Dv4_iS2_i

This is out of specification https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Image_Operands When LoD is not specified in write_imagei instruction then Image Operands must not be specified at all. The correct behavior was before commit https://github.com/KhronosGroup/SPIRV-LLVM-Translator/commit/b3e10f5844a432716a9214ef341499d2861e146b

This regression was undetected because the test https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/llvm_release_140/test/transcoding/OpImageWrite.cl lacks opening brackets in // CHECK-SPV-IR: Should be this: // CHECK-SPV-IR: call spir_func void @_Z18__spirv_ImageWritePU3AS133__spirv_Image__void_2_0_0_0_0_0_1Dv4_iS2_( instead of this: // CHECK-SPV-IR: call spir_func void @_Z18__spirv_ImageWritePU3AS133__spirv_Image__void_2_0_0_0_0_0_1Dv4_iS2_

I see that commit https://github.com/KhronosGroup/SPIRV-LLVM-Translator/commit/b3e10f5844a432716a9214ef341499d2861e146b is backmerge of https://github.com/KhronosGroup/SPIRV-LLVM-Translator/commit/bb4ead0d84870afe1f6f4ce829ba985b1b7950c7 therefore I suspect that this error is also on the main branch. I also suspect that the regression could be on other OpImage* instructions that do not use LoD.

[svenvh]

When LoD is not specified in write_imagei instruction then Image Operands must not be specified at all.

I'm not sure where this restriction comes from, could you point out where that is specified?

Should be this: // CHECK-SPV-IR: call spir_func void @_Z18__spirv_ImageWritePU3AS133__spirv_Image__void_2_0_0_0_0_0_1Dv4_iS2_(

Without the additional Image Operand we can't do a lossless reverse translation. That is, for this call, the translator won't know whether to emit write_imagei or write_imageui. The SignExtend/ZeroExtend Image Operands solve that problem (for SPIR-V >= 1.4).

lacks opening brackets in // CHECK-SPV-IR:

Good point, that's indeed an issue we should fix. But I'm not convinced yet that the extra Image Operand argument is a regression; I would like to see some clarification or opinions from others.

[svenvh]

Just found this in the OpenCL SPIR-V Environment specification:

The image write instruction OpImageWrite must not include any optional Image Operands.

I guess this is what you're referring to? In that case it seems we have an issue indeed. I wonder if the env spec should be updated to allow SignExtend/ZeroExtend Image Operands for OpImageWrite?

Update: I have raised https://github.com/KhronosGroup/OpenCL-Docs/issues/854 for clarification of the env spec.

[mnaczk]

According to https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpImageWrite

If OpImageWrite wants to have optional Image Operands, then it must have a mask and then operands which want to use. But these operands need to be in the order specified by the mask. This is described here. https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Image_Operands

Provides additional operands to sampling, or getting texels from, an image. Bits that are set indicate whether an additional operand follows, as described by the table. If there are multiple following operands indicated, they are ordered: Those indicated by smaller-numbered bits appear first. At least one bit must be set (None is invalid).

Therefore call spir_func void @_Z18__spirv_ImageWritePU3AS133__spirv_Image__void_2_0_0_0_0_0_1Dv4_iS2_i is illegal because it has Image Operands mask but nothing else next.

[svenvh]

Therefore call spir_func void @_Z18__spirv_ImageWritePU3AS133__spirv_Image__void_2_0_0_0_0_0_1Dv4_iS2_i is illegal because it has Image Operands mask but nothing else next.

Whether that's illegal or not depends on the bits that are set in the mask according to my understanding. It's fine if only the Image Operands mask is present when none of the enabled bits in the mask require any following operands. For example, Bias explicitly says that another operand follows:

0x1 Bias A following operand is the bias added to the implicit level of detail.

So if the Image Operands mask has bit 0 set, then there must be another operand after the mask. But the Image Operand SignExtend (which is the case for the test you highlighted, as the argument has value 4096 = 0x1000) does not require any following operands. That makes sense because all the required information is already conveyed by the enabled bit in the mask; there is no need for another operand.

[mnaczk]

You are right. call spir_func void @_Z18__spirv_ImageWritePU3AS133__spirv_Image__void_2_0_0_0_0_0_1Dv4_iS2_i Could be correct when Image Operands is used to set SignExtended. I have to make changes on my project side.

The remaining issue is to improve LIT tests by adding missing open brackets.

[svenvh]

Thanks for confirming. We should indeed leave the issue open until the test checks have been updated. I have made a start in #1659 and already found a few cases where the pattern needs updating.