[Piglit] [PATCH 03/24] framework: add --glsl option

[Alejandro Piñeiro]

On 26/06/18 17:35, Dylan Baker wrote:
> Quoting Alejandro Piñeiro (2018-06-26 03:44:53)
>> On 25/06/18 17:59, Dylan Baker wrote:
>>> Quoting Alejandro Piñeiro (2018-06-23 04:26:33)
>>>> On 22/06/18 19:14, Dylan Baker wrote:
>>>>> Quoting Alejandro Piñeiro (2018-06-20 05:40:38)
>>>>>> So when executing shader tests, they will be executed with -glsl. This
>>>>>> is the force GLSL mode, that is only relevant if the shader test
>>>>>> includes SPIR-V shaders.
>>>>> I'm not sure I understand what you're doing. It looks like you're planning to
>>>>> build tests that can be run in either SPIRV or GLSL mode, that they can be
>>>>> forced into GLSL mode using a switch, or use SPIRV mode if available. Is that
>>>>> correct?
>>>> Yes. Perhaps the commit message is not really clear, as all the details
>>>> are included on the previous commit "shader_runner/spirv: support
>>>> loading SPIR-V shaders". On that commit we explain that we add a -glsl
>>>> option to shader_runner, that is mostly a debugging option. What this
>>>> commits adds is adding the -glsl option when running the tests in a
>>>> batch. With this series, the -glsl option would only make sense with the
>>>> ARB_gl_spirv tests we are adding.
>>>>
>>>> Do you think that I should update the commit message to be more clear?
>>>>
>>>> BR
>>>>
>>> My concern is that you could end up with two tests with the same name that
>>> aren't the same (I think), since the --glsl option won't change the name of the
>>> test.
>> Right now, the idea behind the --glsl option is not getting two tests
>> from the same executable. The tests included with this series are
>> expected to run on SPIR-V mode. So for example, we would not add a new
>> entry on opengl.py/shader.py to run those tests twice. As mentioned it
>> is mostly a debug utility/sanity check, that was useful when we were
>> working on getting them passed, so we understand that will be still
>> useful on the future, for any other driver interested on support the
>> extension, or even for i965 again, if there is  any future regression.
>>
>> Perhaps you are thinking that we plan to use that option, or similar, to
>> reuse tests from other extensions. What I called "borrowed tests" on the
>> RFC I sent some months ago. For that case, our plan would be generate
>> new tests to be placed under generated_tests. So in the case of reusing
>> tests from other extensions, we would still have two different base
>> tests (one for GLSL, other for SPIRV).
>>
>> BR
> That sounds reasonable. My concern was that someone would see tests failing
> because they lacked spirv tools, and run with --glsl, then give that to a second
> person who did have spriv-tools, and see different results.

Well, fwiw, that option is just useful, not totally needed. So if you
are really worried about people not using it properly, we can just drop
this patch. After all, usually when you are debugging, you do it with a
individual test, so it is enough if shader_runner has that option.

So how about that compromise? We keep the -glsl option on shader_runner,
but drop the piglit framework support to run several tests with that option?

BR




-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20180627/501bc300/attachment.sig>