Khronos/spirv 3.6.1 out

[AndreyE]

Fixed a couple of bugs in SPIR-V reader.
1st. Incorrect mangling of atomic_inc and atomic_min/max with unsigned arguments
2nd. Incorrect handling of vector conversions which should be converted into OCL built-ins since there is no vector conversion in OpenCL C/C++

[yxsamliu]

commit 75080bd caused regression. issue #20 opened.

[yxsamliu]

OpBitcast is a conversion instruction.

The issue is that clang may generates conversion instructions with vector as operand which cannot be represented by OCL builtin functions. I think backends should be able to handle these instructions, therefore it is unnecessary to translate them to OCL builtin functions.

[yxsamliu]

This commit also causes other regressions. For example, isequal() returns vector of i32, but in SPIR-V it needs to be translated to an instruction returning vector of bool, so a vector conversion instruction needs to be added, which cannot be translated to OCL builtin functions.

I think we'd better revert this change until these issues fixed.

[bader]

The issue is that clang may generates conversion instructions with vector as operand which cannot be represented by OCL builtin functions.

We should not use OCL built-in functions only for that case. For OpenCL compatible types we should use OpenCL built-in function. In attached to the patch test it's demonstrated that round-trip translation via SPIR-V format replaces _Z13convert_uint4Dv4_f with fptoui, which is incorrect because these operation has different definitions and might be implemented differently.

astype(OpenCL) -> bitcast(LLVM) -> OpBitcast(SPIR-V)
convert*(OpenCL)-> func. call convert (LLVM) -> OpConvert*(SPIR-V) - even if some conversion operations might be implemented via LLVM instructions, using them would be loss of original program semantic.

[yxsamliu]

These OpenCL builtin functions should have the same semantic as the LLVM conversion instructions, since they are translated to the same thing in SPIR-V, otherwise they should be defined as OpenCL extended instructions.

We run OpenCL conformance tests with these conversion builtin functions translated to LLVM conversion instructions and did not see issues.

[bader]

We run OpenCL conformance tests with these conversion builtin functions translated to LLVM conversion instructions and did not see issues.

That proves that your back-end compiler generates OpenCL conformant code for LLVM conversion instructions, but there is a slight difference in the corner case behavior:
LLVM: http://llvm.org/docs/LangRef.html#fptoui-to-instruction If the value cannot fit in ty2, the results are undefined.
OpenCL: 6.2.3.3 When converting from a floating-point type to integer type, the behavior is
implementation-defined.

Using fptoui is a potential bug - undefined behavior allows compiler to do anything it wants with the code.

[yxsamliu]

Undefined behavior and implementation defined behavior do not make much difference here since they are both not portable. If people need defined behavior for the overflow case, they can use the saturated conversion functions, which will always be translated back to OCL builtin functions.

[AndreyE]

I'm sorry for this low quality fix. I was misguided by isCvtOpCode function name, didn't notice it covers bitcast as well. From this moment I'm going to check my commits on a broader set of tests.
Anyway I agree with Alexey what replacing _Z13convert_uint4Dv4_f with fptoui is incorrect. Apart from possible performance losses the vector conversions are not defined in SPIR-1.2/2.0 spec. See ch. 3.3 LLVM Supported Instructions.

[yxsamliu]

Let's not say it was low quality. I failed to see the issue when doing the review either :-)

I don't think replacing _Z13convert_uint4Dv4_f to fptoui can cause performance issues since the only difference is when overflowing happens. On the other hand, using fptoui may have better performance since LLVM knows about it. Do you see any apps or tests fail due to this change?

[AndreyE]

We cannot know this in advance. If a vector conversion is not supported natively by a target hardware it would be emulated. OpenCL adopter should be able to decide what is better to her; improve LLVM CodeGen or optimize the built-in (and maybe keep it undisclosed)

Our internal test fails probably due to different algorithms used.

[yxsamliu]

How about add an option cl::opt SPIRVGenFuncCallForVectorConversion to SPIRVReader and let vendor choose what to generate for vector conversion.

The reason is that there may be other FE which generates vector conversion instructions directly and they don't want them to be translated to OCL builtins.

[AndreyE]

Not sure this option is necessary. Conversion built-ins must be implemented by any OpenCL vendor. Vendor may employ LLVM conversion instructions or its own implementation of these built-ins.

[yxsamliu]

Although currently the SPIR-V/LLVM translator only targets OpenCL runtime, but it may be used for targeting other platforms. If there are two alternative representations, we better have an option for that. Also this may help debugging.

[AndreyE]

IMHO at the moment it is easier to write another spir-v reader which targets different run-time than make the current implementation to support other than OpenCL targets.

[yxsamliu]

I disagree. Large portion of SPIRVReader.cpp are platform independent and could be shared between different platforms. Actually I plan to refactor it to generate SPIR-V friendly format first and separate the OCL specific part as passes.

[AndreyE]

OK, I got it. This means what it is better to make modifying of LLVM vector conversion instructions into calls to OpenCL built-ins should be done elsewhere. Presumably in SPIRVToOCL20 pass.