Aug. 2, 2023
8:44 a.m.
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/1134405930780471326); 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/8e544a3947a67bf20edaf4ad30686b3176b275ce 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`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_41063