arb_compute_variable_group_size: Fix require section.

Submitted by Matt Turner on Oct. 17, 2016, 7:39 p.m.

Details

Message ID 1476733158-2462-1-git-send-email-mattst88@gmail.com
State New
Headers show
Series "arb_compute_variable_group_size: Fix require section." ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Matt Turner Oct. 17, 2016, 7:39 p.m.
---
 .../linker/mixed_fixed_variable_local_work_size.shader_test             | 2 +-
 .../linker/no_local_size_specified.shader_test                          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
index d660d56..32088ad 100644
--- a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
+++ b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
@@ -7,7 +7,7 @@ 
 [require]
 GL >= 3.3
 GLSL >= 3.30
-GL_ARB_compute_shader
+GL_ARB_compute_variable_group_size
 
 [compute shader]
 #version 330
diff --git a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
index 6371c29..92a1646 100644
--- a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
+++ b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
@@ -7,7 +7,7 @@ 
 [require]
 GL >= 3.3
 GLSL >= 3.30
-GL_ARB_compute_shader
+GL_ARB_compute_variable_group_size
 
 [compute shader]
 #version 330

Comments

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

On 2016-10-17 12:39:18, Matt Turner wrote:
> ---
>  .../linker/mixed_fixed_variable_local_work_size.shader_test             | 2 +-
>  .../linker/no_local_size_specified.shader_test                          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
> index d660d56..32088ad 100644
> --- a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
> +++ b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
> @@ -7,7 +7,7 @@
>  [require]
>  GL >= 3.3
>  GLSL >= 3.30
> -GL_ARB_compute_shader
> +GL_ARB_compute_variable_group_size
>  
>  [compute shader]
>  #version 330
> diff --git a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
> index 6371c29..92a1646 100644
> --- a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
> +++ b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
> @@ -7,7 +7,7 @@
>  [require]
>  GL >= 3.3
>  GLSL >= 3.30
> -GL_ARB_compute_shader
> +GL_ARB_compute_variable_group_size
>  
>  [compute shader]
>  #version 330
> -- 
> 2.7.3
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
Thanks for fixing this.

Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>

On 10/17/2016 09:39 PM, Matt Turner wrote:
> ---
>  .../linker/mixed_fixed_variable_local_work_size.shader_test             | 2 +-
>  .../linker/no_local_size_specified.shader_test                          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
> index d660d56..32088ad 100644
> --- a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
> +++ b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
> @@ -7,7 +7,7 @@
>  [require]
>  GL >= 3.3
>  GLSL >= 3.30
> -GL_ARB_compute_shader
> +GL_ARB_compute_variable_group_size
>
>  [compute shader]
>  #version 330
> diff --git a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
> index 6371c29..92a1646 100644
> --- a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
> +++ b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
> @@ -7,7 +7,7 @@
>  [require]
>  GL >= 3.3
>  GLSL >= 3.30
> -GL_ARB_compute_shader
> +GL_ARB_compute_variable_group_size
>
>  [compute shader]
>  #version 330
>
On 10/17/2016 09:45 PM, Samuel Pitoiset wrote:
> Thanks for fixing this.
>
> Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>

Actually, we need to check for both ARB_compute_shader and 
ARB_compute_variable_group_size since 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=8785a8ff8948385a913e9bd75e8cdd1092bd750f.

>
> On 10/17/2016 09:39 PM, Matt Turner wrote:
>> ---
>>  .../linker/mixed_fixed_variable_local_work_size.shader_test
>> | 2 +-
>>  .../linker/no_local_size_specified.shader_test
>> | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git
>> a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
>> b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
>>
>> index d660d56..32088ad 100644
>> ---
>> a/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
>>
>> +++
>> b/tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
>>
>> @@ -7,7 +7,7 @@
>>  [require]
>>  GL >= 3.3
>>  GLSL >= 3.30
>> -GL_ARB_compute_shader
>> +GL_ARB_compute_variable_group_size
>>
>>  [compute shader]
>>  #version 330
>> diff --git
>> a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
>> b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
>>
>> index 6371c29..92a1646 100644
>> ---
>> a/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
>>
>> +++
>> b/tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
>>
>> @@ -7,7 +7,7 @@
>>  [require]
>>  GL >= 3.3
>>  GLSL >= 3.30
>> -GL_ARB_compute_shader
>> +GL_ARB_compute_variable_group_size
>>
>>  [compute shader]
>>  #version 330
>>
On Mon, Oct 17, 2016 at 12:54 PM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
>
> On 10/17/2016 09:45 PM, Samuel Pitoiset wrote:
>>
>> Thanks for fixing this.
>>
>> Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>
>
> Actually, we need to check for both ARB_compute_shader and
> ARB_compute_variable_group_size since
> https://cgit.freedesktop.org/mesa/mesa/commit/?id=8785a8ff8948385a913e9bd75e8cdd1092bd750f.

Strange. Shouldn't a [compute shader] section (or requiring
GL_ARB_compute_variable_group_size) be sufficient?

I'll make the change regardless.
On 10/17/2016 10:33 PM, Matt Turner wrote:
> On Mon, Oct 17, 2016 at 12:54 PM, Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>>
>>
>> On 10/17/2016 09:45 PM, Samuel Pitoiset wrote:
>>>
>>> Thanks for fixing this.
>>>
>>> Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>
>>
>> Actually, we need to check for both ARB_compute_shader and
>> ARB_compute_variable_group_size since
>> https://cgit.freedesktop.org/mesa/mesa/commit/?id=8785a8ff8948385a913e9bd75e8cdd1092bd750f.
>
> Strange. Shouldn't a [compute shader] section (or requiring
> GL_ARB_compute_variable_group_size) be sufficient?

Doesn't seem like.

>
> I'll make the change regardless.

Nicolai also submitted a patch pretty similar to this one, except that 
he checks for both extensions.

>
Quoting Matt Turner (2016-10-17 13:33:11)
> On Mon, Oct 17, 2016 at 12:54 PM, Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
> >
> >
> > On 10/17/2016 09:45 PM, Samuel Pitoiset wrote:
> >>
> >> Thanks for fixing this.
> >>
> >> Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> >
> >
> > Actually, we need to check for both ARB_compute_shader and
> > ARB_compute_variable_group_size since
> > https://cgit.freedesktop.org/mesa/mesa/commit/?id=8785a8ff8948385a913e9bd75e8cdd1092bd750f.
> 
> Strange. Shouldn't a [compute shader] section (or requiring
> GL_ARB_compute_variable_group_size) be sufficient?

No, [computer shader] doesn't imply the requirement. The way shader runner is
implemented it checks the [require] block, then if everything seems good starts
compiling and linking, and doesn't expect errors. So you either need the GL/GLSL
version to imply the extension or to ask for it explicitly.

The fast skipping code in the python layer has the same limitations

Dylan

> 
> I'll make the change regardless.
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
On 17.10.2016 22:33, Matt Turner wrote:
> On Mon, Oct 17, 2016 at 12:54 PM, Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>>
>>
>> On 10/17/2016 09:45 PM, Samuel Pitoiset wrote:
>>>
>>> Thanks for fixing this.
>>>
>>> Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>
>>
>> Actually, we need to check for both ARB_compute_shader and
>> ARB_compute_variable_group_size since
>> https://cgit.freedesktop.org/mesa/mesa/commit/?id=8785a8ff8948385a913e9bd75e8cdd1092bd750f.
>
> Strange. Shouldn't a [compute shader] section (or requiring
> GL_ARB_compute_variable_group_size) be sufficient?
>
> I'll make the change regardless.

They're slightly different things. Your patch made sure that the test is 
skipped when ARB_compute_variable_group_size isn't available. And it's 
true that for the [require] section, you actually don't need to 
explicitly check for ARB_compute_shader on top of that, because a driver 
that advertises ARB_compute_variable_group_size must also advertise 
ARB_compute_shader.

My patch was mostly about adding the `#extension GL_ARB_compute_shader: 
enable` to the shaders themselves. I've pushed that now as well.

Cheers,
Nicolai