[Piglit] [PATCH] glsl-1.30: Reproduce a bug in the i965 backend optimizer

[Ilia Mirkin]

On Thu, Sep 13, 2018 at 8:26 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 09/12/2018 09:17 PM, Ilia Mirkin wrote:
>> On Wed, Sep 12, 2018 at 7:59 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>> On 09/12/2018 04:33 PM, Ilia Mirkin wrote:
>>>> On Wed, Sep 12, 2018 at 7:29 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>>> From: Ian Romanick <ian.d.romanick at 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.romanick at intel.com>
>>>>> Cc: Matt Turner <mattst88 at 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 <imirkin at alum.mit.edu>

pretty much irrespective of where you put it.