On Wed Aug 2 13:44:29 2023 +0000, Zebediah Figura wrote:
Could you please also fix `ftoi`?
Sure. I was more concerned about ftou, since it was one of the last things preventing Wine tests from working, but it shouldn't be too much more work to fix ftoi.
I had written a similar patch at some point. The reason why I
hesitated sending it is that it's not completely clear what "Behavior is undefined" mean in the SPIR-V specification, since what your code does is to compute `ConvertFToU` anyway and then ignore it if the input is not in the range. According to the C/C++ meaning of "undefined behavior" this would be invalid: once you invoke UB there's nothing you can do to "go back", and even if you ignore the computed value the compiler has no obligations any more with you. If SPIR-V adopted the same meaning, which would sound plausible, your implementation (and mine as well) would be incorrect. Do you have any specific insight in this matter? I don't have any better insight than you, and asking for a clarification on what undefined behaviour actually means would be a good thing. I guess there are two questions here: (1) can an "undefined" instruction do anything other than returning an arbitrary value, including cascading to further instructions that use that undefined value in computation? I suspect the answer is, or will be, no, if nothing else then at least for arithmetic instructions. This is at least partially because it is hard for shaders to crash, or corrupt data, in a meaningful sense. I also suspect this is the intent because SPIR-V explicitly includes an OpUndef instruction, which would otherwise mean "please crash my computer". I also suspect this is the case because given the way shaders are designed, it's not really possible to avoid executing an undefined instruction; when branching is encountered shaders will basically execute both branches in parallel invocations, so all of the code gets run anyway. (2) can an "undefined" instruction that is *not* used in computation [as with the OpSelect here] affect the result of that computation? E.g. can "<true> ? <value> : <undef>" yield anything other than <value>? I suspect the answer is again no, although I don't have much better to base this on than "that's the simplest and most intuitive way for shaders to behave", and also again "what would be the point of OpUndef otherwise". I don't *know* that the answers are no, and I think we should still request a spec clarification. But I think an answer of yes to either would make the language near unusable as a compiler target. Fundamentally, if this isn't enough to avoid undefined behaviour, I don't think anything is. And it's worth pointing out that no spec will ever be perfect, and no "conformant" implementation will be perfect either, and this behaviour seems sensible, and fixes a problem in a way that aligns with what the spec's intent *looks* like. Also, this isn't the first time that we've avoided undefined behaviour with an OpSelect—see again udiv.
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`.