<div dir="ltr">On 25 January 2013 07:49, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="im">On 24 January 2013 19:44, Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Following this email are eight patches that add the 4x8 pack/unpack<br>
operations that are the difference between what GLSL ES 3.0 and<br>
ARB_shading_language_packing require.<br>
<br>
They require Chad's gles3-glsl-packing series and are available at<br>
<a href="http://cgit.freedesktop.org/~mattst88/mesa/log/?h=ARB_shading_language_packing" target="_blank">http://cgit.freedesktop.org/~mattst88/mesa/log/?h=ARB_shading_language_packing</a><br>
<br>
I've also added testing support on top of Chad's piglit patch. The<br>
{vs,fs}-unpackUnorm4x8 tests currently fail, and I've been unable to<br>
spot why.<br></blockquote><div><br></div></div><div>I had minor comments on patches 4/8 and 5/8. The remainder is:<br><br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>><br>
<br></div><div>I didn't spot anything that would explain the failure in unpackUnorm4x8 tests. I'll go have a look at your piglit tests now, and if I don't find anything there either, I'll fire up the simulator and see if I can see what's going wrong.<br>
</div></div></div></div></blockquote><div><br></div><div>I found the problem. On i965, floating point divisions are implemented as multiplication by a reciprocal, whereas on the CPU there's a floating point division instruction. Therefore, unpackUnorm4x8's computation of "f / 255.0" doesn't yield consistent results when run on the CPU vs the GPU--there is a tiny difference due to the accumulation of floating point rounding errors.<br>
<br>That's why the "fs" and "vs" variants of the tests failed, and the "const" variant passed--because Mesa does constant folding using the CPU's floating point division instruction, which matches the Python test generator perfectly, whereas the "fs" and "vs" variants use the actual GPU.<br>
<br></div><div>It's only by dumb luck that this rounding error issue didn't bite us until now, because in principle it could equally well have occurred in the unpack2x16 functions.<br></div><div><br></div><div>I believe we should relax the test to allow for these tiny rounding errors (this is what the other test generators, such as gen_builtin_uniform_tests.py, do). As an experiment I modified gen_builtin_packing_tests.py so that in vs_unpack_2x16_template and fs_unpack_2x16_template, "actual == expect${j}" is replaced with "distance(actual, expect${j}) < 0.00001". With this change, the test passes.<br>
<br>However, that change isn't good enough to commit to piglit, for two reasons:<br><br></div><div>(1) It should only be applied when testing the functions whose definition includes a division (unpackUnorm4x8, unpackSnorm4x8, unpackUnorm2x16, and unpackSnorm2x16). A properly functioning implementation ought to be able to get exact answers with all the other packing functions, and we should test that it does.<br>
<br></div><div>(2) IMHO we should test that ouput values of 0.0 and +/- 1.0 are produced without error, since a shader author might conceivably write code that relies on these values being exact. That is, we should check that the following conversions are exact, with no rounding error:<br>
<br>unpackUnorm4x8(0) == vec4(0.0)<br>unpackUnorm4x8(0xffffffff) == vec4(1.0)<br>unpackSnorm4x8(0) == vec4(0.0)<br>unpackSnorm4x8(0x7f7f7f7f) == vec4(1.0)<br>unpackSnorm4x8(0x80808080) == vec4(-1.0)<br>unpackSnorm4x8(0x81818181) == vec4(-1.0)<br>
unpackUnorm2x16(0) == vec2(0.0)<br>unpackUnorm2x16(0xffffffff) == vec4(1.0)<br>unpackSnorm2x16(0) == vec4(0.0)<br>unpackSnorm2x16(0x7fff7fff) == vec4(1.0)<br>unpackSnorm2x16(0x80008000) == vec4(-1.0)<br>unpackSnorm2x16(0x80018001) == vec4(-1.0)<br>
<br></div><div>My recommendation: address problem (1) by modifying the templates to accept a new parameter that determines whether the test needs to be precise or approximate (e.g. "func.precise"). Address problem (2) by hand-coding a few shader_runner tests to check the cases above. IMHO it would be ok to leave the current patch as is (modulo my previous comments) and do a pair of follow-on patches to address problems (1) and (2).<br>
<br></div><div>Chad, do you have any thoughts on this subject, since you're the original author of this test generator?<br></div></div></div></div>