On Thu, Sep 13, 2018 at 8:26 PM, Ian Romanick <i...@freedesktop.org> wrote: > On 09/12/2018 09:17 PM, Ilia Mirkin wrote: >> On Wed, Sep 12, 2018 at 7:59 PM, Ian Romanick <i...@freedesktop.org> wrote: >>> On 09/12/2018 04:33 PM, Ilia Mirkin wrote: >>>> On Wed, Sep 12, 2018 at 7:29 PM, Ian Romanick <i...@freedesktop.org> wrote: >>>>> From: Ian Romanick <ian.d.roman...@intel.com> >>>>> >>>>> The optimizer recently added the ability to replace a compare with a >>>>> subtraction under certain circumstances. This can fail for integers. >>>>> For inputs a = 0x80000000, b = 4, int(0x80000000) < 4, but >>>>> int(0x80000000) - 4 overflows and results in 0x7ffffffc. That's not >>>>> less than zero, so the flags get set differently than for (a < b). >>>>> >>>>> This really only affected the signed comparisons because the subtract >>>>> would always have a signed source types, so it wouldn't be seen as a >>>>> match for the compare with unsigned source types. >>>>> >>>>> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> >>>>> Cc: Matt Turner <matts...@gmail.com> >>>>> --- >>>>> tests/spec/CMakeLists.txt | 1 + >>>>> .../fs-absoluteDifference-int.shader_test | 85 >>>>> +++++++++++++++++++ >>>>> .../fs-absoluteDifference-uint.shader_test | 85 >>>>> +++++++++++++++++++ >>>>> .../vs-absoluteDifference-int.shader_test | 96 >>>>> ++++++++++++++++++++++ >>>>> .../vs-absoluteDifference-uint.shader_test | 96 >>>>> ++++++++++++++++++++++ >>>>> 5 files changed, 363 insertions(+) >>>>> create mode 100644 >>>>> tests/spec/glsl-1.30/execution/fs-absoluteDifference-int.shader_test >>>>> create mode 100644 >>>>> tests/spec/glsl-1.30/execution/fs-absoluteDifference-uint.shader_test >>>>> create mode 100644 >>>>> tests/spec/glsl-1.30/execution/vs-absoluteDifference-int.shader_test >>>>> create mode 100644 >>>>> tests/spec/glsl-1.30/execution/vs-absoluteDifference-uint.shader_test >>>>> >>>>> diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt >>>>> index 4df9d331d..28abf3634 100644 >>>>> --- a/tests/spec/CMakeLists.txt >>>>> +++ b/tests/spec/CMakeLists.txt >>>>> @@ -2,6 +2,7 @@ add_subdirectory (amd_framebuffer_multisample_advanced) >>>>> add_subdirectory (amd_depth_clamp_separate) >>>>> add_subdirectory (amd_performance_monitor) >>>>> add_subdirectory (amd_pinned_memory) >>>>> +add_subdirectory (amd_transform_feedback3_lines_triangles)
This seems out of place, btw... >>>>> add_subdirectory (arb_arrays_of_arrays) >>>>> add_subdirectory (arb_base_instance) >>>>> add_subdirectory (arb_bindless_texture) >>>>> diff --git >>>>> a/tests/spec/glsl-1.30/execution/fs-absoluteDifference-int.shader_test >>>>> b/tests/spec/glsl-1.30/execution/fs-absoluteDifference-int.shader_test >>>>> new file mode 100644 >>>>> index 000000000..cdac53cdf >>>>> --- /dev/null >>>>> +++ b/tests/spec/glsl-1.30/execution/fs-absoluteDifference-int.shader_test >>>>> @@ -0,0 +1,85 @@ >>>>> +[require] >>>>> +GL >= 3.0 >>>>> +GLSL >= 1.30 >>>>> + >>>>> +[vertex shader passthrough] >>>>> + >>>>> +[fragment shader] >>>>> +#extension GL_EXT_shader_integer_mix: enable >>>>> + >>>>> +// { A, B, absoluteDifference(A, B) } >>>>> +uniform ivec3 data[40]; >>>>> + >>>>> +out vec4 color; >>>>> + >>>>> +uint abs_diff(int a, int b) >>>>> +{ >>>>> + /* This can fail if the compiler replaces the (a < b) with the >>>>> result of >>>>> + * one of the subtractions. For inputs a = 0x80000000, b = 4, >>>>> + * int(0x80000000) < 4, but int(0x80000000)-4 overflows and results >>>>> in >>>>> + * 0x7ffffffc. That's not less than zero, so the flags get set >>>>> + * differently than for (a < b). >>>>> + */ >>>>> +#ifndef GL_EXT_shader_integer_mix >>>>> + return (a < b) ? uint(b - a) : uint(a - b); >>>>> +#else >>>>> + return mix(uint(a - b), uint(b - a), a < b); >>>>> +#endif >>>> >>>> Why bother with this? It'll end up testing different code depending on >>>> the extensions that the driver supports, which seems like a really bad >>>> idea. >>> >>> We only hit the bug in the mix() case because how how we lower flow >>> control to bcsel. I thought about having it require >>> GL_EXT_shader_integer_mix. That means that when a driver enabled that >>> extension the test could transition SKIP->FAIL which seems more likely >>> to be ignored than PASS->FAIL. I guess now there's a possible >>> FAIL->PASS transition. Meh. >>> >>> I'm not married to it being either way. I just want to reproduce the >>> bug that I added to the i965 driver a few months ago. :) >> >> Hmm. Meh indeed. I dunno though -- skip -> fail when enabling an ext >> seems pretty bad to me. I'd just require EXT_shader_integer_mix. In >> practice, it's enabled everywhere GL 3.0 is (in mesa), so it's all a >> bit academic. > > Fair enough. The bikeshedder in me has difficulty deciding where the > tests should live. It requires extensions beyond GLSL 1.30, so > spec/glsl-1.30 doesn't seem good. It's not really a > GL_EXT_shader_integer_mix test, so spec/ext_shader_integer_mix doesn't > seem good either. :) tests/random/ :) That should make your OCD explode... anyways, with the #ifdef and spurious add_subdirectory removed, this is all Reviewed-by: Ilia Mirkin <imir...@alum.mit.edu> pretty much irrespective of where you put it. _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit