Re: [Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN
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
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
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
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
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
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
[Mesa-dev] [PATCH] i965: Add gl_state_index casts for PATCH_VERTICES_IN
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 = -- 2.5.0.400.gff86faf ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev