On Wed Aug 2 13:44:29 2023 +0000, Giovanni Mascellani wrote:
After some research, my feeling is different than yours WRT the two questions you asked. Specifically:
- [I asked on a Khronos-managed Discord instance my
question](https://discord.com/channels/427551838099996672/427951557045387276/113440593...); the only answer I got is that the C/C++ interpretation is valid, meaning that as soon as you invoke UB anything can happen: the program can change arbitrarily, the driver can declare the device lost, etc. Granted, that's just one random user giving their opinion, so it's nothing more than a data point.
- The comment for `udiv` mentions that the RISC-V spec says: "The
resulting value is undefined if Operand 2 is 0". That's not the spec text anymore: in commit https://gitlab.khronos.org/spirv/SPIR-V/-/commit/8e544a3947a67bf20edaf4ad306... it was changed to "Behavior is undefined if Operand 2 is 0", which is a stronger formulation (the same used for `OpConvertFToU`). Also, while the SPIR-V spec has in principle no relationship with the C/C++ specs, the fact that they decided to switch to the same formulation reinforces my belief that the same meaning is sought. Also, C is clearly the dominant programming language in this software engineering segment, so the interpretation is sensible anyway.
- The fact that GPUs are going to execute both branches anyway to then
select the relevant result is, IMHO, irrelevant here; it's just an implementation detail. There are many other effects that can happen if the generated code is UB (in the C/C++ sense), like a compiler inferring other unwanted optimizations. So my interpretation is that to emit correct SPIR-V code we should use a full `OpBranchConditional`, so that `OpConvertFToU` is not even executed when outside of its domain. OTOH, I don't think that necessarily has to happen within this MR, which I consider an improvement over the current situation anyway, at least from the correctness point of view. I don't know how to weigh overhead, really, though I tend to agree with you on the count we already have a similar trick for `udiv`.
To be fair, I should say it's quite possible that was the intention of "undefined behaviour" in general. But in the case of arithmetic instructions, I find it surprising that they would really have completely undefined behaviour in practice, or even that that was intended in the specification.
I suspect the switch to consistent wording "undefined behaviour" was to specifically contrast it with "invalid shader module", which has clearly defined semantics—passing an invalid module to Vulkan APIs is illegal (undefined behaviour according to Vulkan).
So my interpretation is that to emit correct SPIR-V code we should use a full `OpBranchConditional`, so that `OpConvertFToU` is not even executed when outside of its domain.
It's worse than that; we'd have to perform a branch *per component*.
I find it hard to believe this was intended. I find it easier to believe that "illegal under any conditions; you'd better make sure your inputs are sanitary" was intended (as problematic as it is), or that "arithmetic instructions produce an undefined value, and you won't have any bad behaviour as long as that value never gets written, so OpSelect is fine" was intended. Of course, I find it even easier to believe that not that much thought was put into "UB" in the first place.