Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN

2018-02-14 Thread Kenneth Graunke 
On Wednesday, February 14, 2018 10:17:07 AM PST Jason Ekstrand wrote:
> On Wed, Feb 14, 2018 at 10:12 AM, Kenneth Graunke 
> wrote:
> 
> > On Wednesday, February 14, 2018 6:05:22 AM PST Marek Olšák wrote:
> > > On Wed, Feb 14, 2018 at 7:57 AM, Kenneth Graunke 
> > wrote:
> > > > On Tuesday, February 13, 2018 2:57:07 PM PST Jason Ekstrand wrote:
> > > >> This fixes the build in clang
> > > >> ---
> > > >>  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 3 ++-
> > > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > > >> index 10a4ff4..69da83a 100644
> > > >> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > > >> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > > >> @@ -254,7 +254,8 @@ 
> > > >> brw_nir_lower_patch_vertices_in_to_uniform(nir_shader
> > *nir)
> > > >>gl_state_index16 tokens[STATE_LENGTH] = {
> > > >>   STATE_INTERNAL,
> > > >>   nir->info.stage == MESA_SHADER_TESS_CTRL ?
> > > >> -STATE_TCS_PATCH_VERTICES_IN :
> > STATE_TES_PATCH_VERTICES_IN,
> > > >> +(gl_state_index16)STATE_TCS_PATCH_VERTICES_IN :
> > > >> +(gl_state_index16)STATE_TES_PATCH_VERTICES_IN,
> > > >>};
> > > >>var->num_state_slots = 1;
> > > >>var->state_slots =
> > > >>
> > > >
> > > > This is fine, but I prefer your plan from IRC:
> > > >
> > > > 1. Add STATE_MAX_VALUE = 0x to the enum.
> > > > 2. Mark the enum PACKED.
> > > > 3. Drop gl_state_index16 again, since gl_state_index should now be the
> > > >desired size, and also an actual enum so it follows the actual C++
> > > >enum rules.
> > > >
> > > > I suppose the downside is that it could cause "case not handled in
> > > > switch" warnings...
> > >
> > > While I don't care too much about how gl_state_index is done,
> > > gl_state_index can also contain arbitrary 16-bit signed integer
> > > values, i.e. it's not strictly enum. And yes, I said "signed".
> > >
> > > Marek
> >
> > Oh. :(  I guess using short or int16_t is pretty reasonable then...
> >
> 
> Does that mean we're happy with this patch?

Yeah, sorry, should have been clearer:

Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN

2018-02-14 Thread Jason Ekstrand 
On Wed, Feb 14, 2018 at 10:12 AM, Kenneth Graunke 
wrote:

> On Wednesday, February 14, 2018 6:05:22 AM PST Marek Olšák wrote:
> > On Wed, Feb 14, 2018 at 7:57 AM, Kenneth Graunke 
> wrote:
> > > On Tuesday, February 13, 2018 2:57:07 PM PST Jason Ekstrand wrote:
> > >> This fixes the build in clang
> > >> ---
> > >>  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > >> index 10a4ff4..69da83a 100644
> > >> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > >> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> > >> @@ -254,7 +254,8 @@ brw_nir_lower_patch_vertices_in_to_uniform(nir_shader
> *nir)
> > >>gl_state_index16 tokens[STATE_LENGTH] = {
> > >>   STATE_INTERNAL,
> > >>   nir->info.stage == MESA_SHADER_TESS_CTRL ?
> > >> -STATE_TCS_PATCH_VERTICES_IN :
> STATE_TES_PATCH_VERTICES_IN,
> > >> +(gl_state_index16)STATE_TCS_PATCH_VERTICES_IN :
> > >> +(gl_state_index16)STATE_TES_PATCH_VERTICES_IN,
> > >>};
> > >>var->num_state_slots = 1;
> > >>var->state_slots =
> > >>
> > >
> > > This is fine, but I prefer your plan from IRC:
> > >
> > > 1. Add STATE_MAX_VALUE = 0x to the enum.
> > > 2. Mark the enum PACKED.
> > > 3. Drop gl_state_index16 again, since gl_state_index should now be the
> > >desired size, and also an actual enum so it follows the actual C++
> > >enum rules.
> > >
> > > I suppose the downside is that it could cause "case not handled in
> > > switch" warnings...
> >
> > While I don't care too much about how gl_state_index is done,
> > gl_state_index can also contain arbitrary 16-bit signed integer
> > values, i.e. it's not strictly enum. And yes, I said "signed".
> >
> > Marek
>
> Oh. :(  I guess using short or int16_t is pretty reasonable then...
>

Does that mean we're happy with this patch?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN

2018-02-14 Thread Kenneth Graunke 
On Wednesday, February 14, 2018 6:05:22 AM PST Marek Olšák wrote:
> On Wed, Feb 14, 2018 at 7:57 AM, Kenneth Graunke  
> wrote:
> > On Tuesday, February 13, 2018 2:57:07 PM PST Jason Ekstrand wrote:
> >> This fixes the build in clang
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp 
> >> b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> >> index 10a4ff4..69da83a 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> >> @@ -254,7 +254,8 @@ brw_nir_lower_patch_vertices_in_to_uniform(nir_shader 
> >> *nir)
> >>gl_state_index16 tokens[STATE_LENGTH] = {
> >>   STATE_INTERNAL,
> >>   nir->info.stage == MESA_SHADER_TESS_CTRL ?
> >> -STATE_TCS_PATCH_VERTICES_IN : STATE_TES_PATCH_VERTICES_IN,
> >> +(gl_state_index16)STATE_TCS_PATCH_VERTICES_IN :
> >> +(gl_state_index16)STATE_TES_PATCH_VERTICES_IN,
> >>};
> >>var->num_state_slots = 1;
> >>var->state_slots =
> >>
> >
> > This is fine, but I prefer your plan from IRC:
> >
> > 1. Add STATE_MAX_VALUE = 0x to the enum.
> > 2. Mark the enum PACKED.
> > 3. Drop gl_state_index16 again, since gl_state_index should now be the
> >desired size, and also an actual enum so it follows the actual C++
> >enum rules.
> >
> > I suppose the downside is that it could cause "case not handled in
> > switch" warnings...
> 
> While I don't care too much about how gl_state_index is done,
> gl_state_index can also contain arbitrary 16-bit signed integer
> values, i.e. it's not strictly enum. And yes, I said "signed".
> 
> Marek

Oh. :(  I guess using short or int16_t is pretty reasonable then...


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN

2018-02-14 Thread Marek Olšák 
On Wed, Feb 14, 2018 at 7:57 AM, Kenneth Graunke  wrote:
> On Tuesday, February 13, 2018 2:57:07 PM PST Jason Ekstrand wrote:
>> This fixes the build in clang
>> ---
>>  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp 
>> b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>> index 10a4ff4..69da83a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>> @@ -254,7 +254,8 @@ brw_nir_lower_patch_vertices_in_to_uniform(nir_shader 
>> *nir)
>>gl_state_index16 tokens[STATE_LENGTH] = {
>>   STATE_INTERNAL,
>>   nir->info.stage == MESA_SHADER_TESS_CTRL ?
>> -STATE_TCS_PATCH_VERTICES_IN : STATE_TES_PATCH_VERTICES_IN,
>> +(gl_state_index16)STATE_TCS_PATCH_VERTICES_IN :
>> +(gl_state_index16)STATE_TES_PATCH_VERTICES_IN,
>>};
>>var->num_state_slots = 1;
>>var->state_slots =
>>
>
> This is fine, but I prefer your plan from IRC:
>
> 1. Add STATE_MAX_VALUE = 0x to the enum.
> 2. Mark the enum PACKED.
> 3. Drop gl_state_index16 again, since gl_state_index should now be the
>desired size, and also an actual enum so it follows the actual C++
>enum rules.
>
> I suppose the downside is that it could cause "case not handled in
> switch" warnings...

While I don't care too much about how gl_state_index is done,
gl_state_index can also contain arbitrary 16-bit signed integer
values, i.e. it's not strictly enum. And yes, I said "signed".

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN

2018-02-13 Thread Kenneth Graunke 
On Tuesday, February 13, 2018 2:57:07 PM PST Jason Ekstrand wrote:
> This fixes the build in clang
> ---
>  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp 
> b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> index 10a4ff4..69da83a 100644
> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> @@ -254,7 +254,8 @@ brw_nir_lower_patch_vertices_in_to_uniform(nir_shader 
> *nir)
>gl_state_index16 tokens[STATE_LENGTH] = {
>   STATE_INTERNAL,
>   nir->info.stage == MESA_SHADER_TESS_CTRL ?
> -STATE_TCS_PATCH_VERTICES_IN : STATE_TES_PATCH_VERTICES_IN,
> +(gl_state_index16)STATE_TCS_PATCH_VERTICES_IN :
> +(gl_state_index16)STATE_TES_PATCH_VERTICES_IN,
>};
>var->num_state_slots = 1;
>var->state_slots =
> 

This is fine, but I prefer your plan from IRC:

1. Add STATE_MAX_VALUE = 0x to the enum.
2. Mark the enum PACKED.
3. Drop gl_state_index16 again, since gl_state_index should now be the
   desired size, and also an actual enum so it follows the actual C++
   enum rules.

I suppose the downside is that it could cause "case not handled in
switch" warnings...

--Ken


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN

2018-02-13 Thread Tapani Pälli 

Reviewed-by: Tapani Pälli 

On 14.02.2018 00:57, Jason Ekstrand wrote:

This fixes the build in clang
---
  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp 
b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
index 10a4ff4..69da83a 100644
--- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
+++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
@@ -254,7 +254,8 @@ brw_nir_lower_patch_vertices_in_to_uniform(nir_shader *nir)
gl_state_index16 tokens[STATE_LENGTH] = {
   STATE_INTERNAL,
   nir->info.stage == MESA_SHADER_TESS_CTRL ?
-STATE_TCS_PATCH_VERTICES_IN : STATE_TES_PATCH_VERTICES_IN,
+(gl_state_index16)STATE_TCS_PATCH_VERTICES_IN :
+(gl_state_index16)STATE_TES_PATCH_VERTICES_IN,
};
var->num_state_slots = 1;
var->state_slots =


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev