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

[Dylan Baker]

Quoting Alejandro Piñeiro (2018-06-27 03:43:35)
> 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.

I'm fine with it as is, if I asked for a change what I think I'd ask for is that
the -glsl option would change the name of the test so that if you compare them
they don't line up. But I don't think that's going to be a real issue, and you
can leave it as-is.

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20180627/20dc25e7/attachment.sig>