Re: [Mesa-dev] [PATCH 2/7] nir: Add a nir_lower_io flag for using load_interpolated_input intrins.

[Kenneth Graunke]

On Tuesday, July 19, 2016 12:57:23 PM PDT Jason Ekstrand wrote:
> On Mon, Jul 18, 2016 at 10:00 PM, Chris Forbes  wrote:
> 
> > Seems a little unfortunate to add a random bool to this interface which is
> > otherwise fairly descriptive, but OK.
> >
> 
> I agree that this is a bit unfortunate.  I was going to suggest adding a
> flags parameter and a bitfield union but a flags parameter for a single
> bool is also kind-of stupid.  I vote we keep it as-is and make it a flags
> parameter once we have 2 bools.
> 
> --Jason

I agree, this sucks.  I've dropped this patch locally in favor of adding
a nir_shader_compiler_options::use_interpolated_input_intrinsics flag
(which I just folded into patch 3 as it's a tiny amount of code).

That was really easy to hook up and is a lot cleaner.


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 2/7] nir: Add a nir_lower_io flag for using load_interpolated_input intrins.

[Jason Ekstrand]

On Mon, Jul 18, 2016 at 10:00 PM, Chris Forbes  wrote:

> Seems a little unfortunate to add a random bool to this interface which is
> otherwise fairly descriptive, but OK.
>

I agree that this is a bit unfortunate.  I was going to suggest adding a
flags parameter and a bitfield union but a flags parameter for a single
bool is also kind-of stupid.  I vote we keep it as-is and make it a flags
parameter once we have 2 bools.

--Jason


>
> On Tue, Jul 19, 2016 at 8:26 AM, Kenneth Graunke 
> wrote:
>
>> While my intention is that the new intrinsics should be usable by all
>> drivers, we need to make them optional until all drivers switch.
>>
>> This doesn't do anything yet, but I added it as a separate patch to
>> keep the interface churn separate for easier review.
>>
>> Signed-off-by: Kenneth Graunke 
>> ---
>>  src/compiler/nir/nir.h  |  3 ++-
>>  src/compiler/nir/nir_lower_io.c | 15 +++
>>  src/gallium/drivers/freedreno/ir3/ir3_cmdline.c |  2 +-
>>  src/mesa/drivers/dri/i965/brw_blorp.c   |  2 +-
>>  src/mesa/drivers/dri/i965/brw_nir.c | 18 +-
>>  src/mesa/drivers/dri/i965/brw_program.c |  4 ++--
>>  src/mesa/state_tracker/st_glsl_to_nir.cpp   |  2 +-
>>  7 files changed, 27 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>> index ac11998..e996e0e 100644
>> --- a/src/compiler/nir/nir.h
>> +++ b/src/compiler/nir/nir.h
>> @@ -2324,7 +2324,8 @@ void nir_assign_var_locations(struct exec_list
>> *var_list, unsigned *size,
>>
>>  void nir_lower_io(nir_shader *shader,
>>nir_variable_mode modes,
>> -  int (*type_size)(const struct glsl_type *));
>> +  int (*type_size)(const struct glsl_type *),
>> +  bool use_load_interpolated_input_intrinsics);
>>  nir_src *nir_get_io_offset_src(nir_intrinsic_instr *instr);
>>  nir_src *nir_get_io_vertex_index_src(nir_intrinsic_instr *instr);
>>
>> diff --git a/src/compiler/nir/nir_lower_io.c
>> b/src/compiler/nir/nir_lower_io.c
>> index b05a73f..aa8a517 100644
>> --- a/src/compiler/nir/nir_lower_io.c
>> +++ b/src/compiler/nir/nir_lower_io.c
>> @@ -39,6 +39,7 @@ struct lower_io_state {
>> void *mem_ctx;
>> int (*type_size)(const struct glsl_type *type);
>> nir_variable_mode modes;
>> +   bool use_interpolated_input;
>>  };
>>
>>  void
>> @@ -394,7 +395,8 @@ nir_lower_io_block(nir_block *block,
>>  static void
>>  nir_lower_io_impl(nir_function_impl *impl,
>>nir_variable_mode modes,
>> -  int (*type_size)(const struct glsl_type *))
>> +  int (*type_size)(const struct glsl_type *),
>> +  bool use_interpolated_input)
>>  {
>> struct lower_io_state state;
>>
>> @@ -402,6 +404,7 @@ nir_lower_io_impl(nir_function_impl *impl,
>> state.mem_ctx = ralloc_parent(impl);
>> state.modes = modes;
>> state.type_size = type_size;
>> +   state.use_interpolated_input = use_interpolated_input;
>>
>> nir_foreach_block(block, impl) {
>>nir_lower_io_block(block, );
>> @@ -413,11 +416,15 @@ nir_lower_io_impl(nir_function_impl *impl,
>>
>>  void
>>  nir_lower_io(nir_shader *shader, nir_variable_mode modes,
>> - int (*type_size)(const struct glsl_type *))
>> + int (*type_size)(const struct glsl_type *),
>> + bool use_interpolated_input)
>>  {
>> nir_foreach_function(function, shader) {
>> -  if (function->impl)
>> - nir_lower_io_impl(function->impl, modes, type_size);
>> +  if (function->impl) {
>> + nir_lower_io_impl(function->impl, modes, type_size,
>> +   use_interpolated_input &&
>> +   shader->stage == MESA_SHADER_FRAGMENT);
>> +  }
>> }
>>  }
>>
>> diff --git a/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
>> b/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
>> index 41532fc..a8a8c1b 100644
>> --- a/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
>> +++ b/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
>> @@ -93,7 +93,7 @@ load_glsl(unsigned num_files, char* const* files,
>> gl_shader_stage stage)
>> // TODO nir_assign_var_locations??
>>
>> NIR_PASS_V(nir, nir_lower_system_values);
>> -   NIR_PASS_V(nir, nir_lower_io, nir_var_all, st_glsl_type_size);
>> +   NIR_PASS_V(nir, nir_lower_io, nir_var_all, st_glsl_type_size,
>> false);
>> NIR_PASS_V(nir, nir_lower_samplers, prog);
>>
>> return nir;
>> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
>> b/src/mesa/drivers/dri/i965/brw_blorp.c
>> index 282a5b2..0473cfe 100644
>> --- a/src/mesa/drivers/dri/i965/brw_blorp.c
>> +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
>> @@ -209,7 +209,7 @@ brw_blorp_compile_nir_shader(struct brw_context *brw,
>> struct nir_shader *nir,
>>unsigned end = var->data.location 

Re: [Mesa-dev] [PATCH 2/7] nir: Add a nir_lower_io flag for using load_interpolated_input intrins.

[Chris Forbes]

Seems a little unfortunate to add a random bool to this interface which is
otherwise fairly descriptive, but OK.

On Tue, Jul 19, 2016 at 8:26 AM, Kenneth Graunke 
wrote:

> While my intention is that the new intrinsics should be usable by all
> drivers, we need to make them optional until all drivers switch.
>
> This doesn't do anything yet, but I added it as a separate patch to
> keep the interface churn separate for easier review.
>
> Signed-off-by: Kenneth Graunke 
> ---
>  src/compiler/nir/nir.h  |  3 ++-
>  src/compiler/nir/nir_lower_io.c | 15 +++
>  src/gallium/drivers/freedreno/ir3/ir3_cmdline.c |  2 +-
>  src/mesa/drivers/dri/i965/brw_blorp.c   |  2 +-
>  src/mesa/drivers/dri/i965/brw_nir.c | 18 +-
>  src/mesa/drivers/dri/i965/brw_program.c |  4 ++--
>  src/mesa/state_tracker/st_glsl_to_nir.cpp   |  2 +-
>  7 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index ac11998..e996e0e 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2324,7 +2324,8 @@ void nir_assign_var_locations(struct exec_list
> *var_list, unsigned *size,
>
>  void nir_lower_io(nir_shader *shader,
>nir_variable_mode modes,
> -  int (*type_size)(const struct glsl_type *));
> +  int (*type_size)(const struct glsl_type *),
> +  bool use_load_interpolated_input_intrinsics);
>  nir_src *nir_get_io_offset_src(nir_intrinsic_instr *instr);
>  nir_src *nir_get_io_vertex_index_src(nir_intrinsic_instr *instr);
>
> diff --git a/src/compiler/nir/nir_lower_io.c
> b/src/compiler/nir/nir_lower_io.c
> index b05a73f..aa8a517 100644
> --- a/src/compiler/nir/nir_lower_io.c
> +++ b/src/compiler/nir/nir_lower_io.c
> @@ -39,6 +39,7 @@ struct lower_io_state {
> void *mem_ctx;
> int (*type_size)(const struct glsl_type *type);
> nir_variable_mode modes;
> +   bool use_interpolated_input;
>  };
>
>  void
> @@ -394,7 +395,8 @@ nir_lower_io_block(nir_block *block,
>  static void
>  nir_lower_io_impl(nir_function_impl *impl,
>nir_variable_mode modes,
> -  int (*type_size)(const struct glsl_type *))
> +  int (*type_size)(const struct glsl_type *),
> +  bool use_interpolated_input)
>  {
> struct lower_io_state state;
>
> @@ -402,6 +404,7 @@ nir_lower_io_impl(nir_function_impl *impl,
> state.mem_ctx = ralloc_parent(impl);
> state.modes = modes;
> state.type_size = type_size;
> +   state.use_interpolated_input = use_interpolated_input;
>
> nir_foreach_block(block, impl) {
>nir_lower_io_block(block, );
> @@ -413,11 +416,15 @@ nir_lower_io_impl(nir_function_impl *impl,
>
>  void
>  nir_lower_io(nir_shader *shader, nir_variable_mode modes,
> - int (*type_size)(const struct glsl_type *))
> + int (*type_size)(const struct glsl_type *),
> + bool use_interpolated_input)
>  {
> nir_foreach_function(function, shader) {
> -  if (function->impl)
> - nir_lower_io_impl(function->impl, modes, type_size);
> +  if (function->impl) {
> + nir_lower_io_impl(function->impl, modes, type_size,
> +   use_interpolated_input &&
> +   shader->stage == MESA_SHADER_FRAGMENT);
> +  }
> }
>  }
>
> diff --git a/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
> b/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
> index 41532fc..a8a8c1b 100644
> --- a/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
> +++ b/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
> @@ -93,7 +93,7 @@ load_glsl(unsigned num_files, char* const* files,
> gl_shader_stage stage)
> // TODO nir_assign_var_locations??
>
> NIR_PASS_V(nir, nir_lower_system_values);
> -   NIR_PASS_V(nir, nir_lower_io, nir_var_all, st_glsl_type_size);
> +   NIR_PASS_V(nir, nir_lower_io, nir_var_all, st_glsl_type_size,
> false);
> NIR_PASS_V(nir, nir_lower_samplers, prog);
>
> return nir;
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> b/src/mesa/drivers/dri/i965/brw_blorp.c
> index 282a5b2..0473cfe 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> @@ -209,7 +209,7 @@ brw_blorp_compile_nir_shader(struct brw_context *brw,
> struct nir_shader *nir,
>unsigned end = var->data.location +
> nir_uniform_type_size(var->type);
>nir->num_uniforms = MAX2(nir->num_uniforms, end);
> }
> -   nir_lower_io(nir, nir_var_uniform, nir_uniform_type_size);
> +   nir_lower_io(nir, nir_var_uniform, nir_uniform_type_size, false);
>
> const unsigned *program =
>brw_compile_fs(compiler, brw, mem_ctx, wm_key, _prog_data, nir,
> diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
> b/src/mesa/drivers/dri/i965/brw_nir.c
> index 

[Mesa-dev] [PATCH 2/7] nir: Add a nir_lower_io flag for using load_interpolated_input intrins.

[Kenneth Graunke]

While my intention is that the new intrinsics should be usable by all
drivers, we need to make them optional until all drivers switch.

This doesn't do anything yet, but I added it as a separate patch to
keep the interface churn separate for easier review.

Signed-off-by: Kenneth Graunke 
---
 src/compiler/nir/nir.h  |  3 ++-
 src/compiler/nir/nir_lower_io.c | 15 +++
 src/gallium/drivers/freedreno/ir3/ir3_cmdline.c |  2 +-
 src/mesa/drivers/dri/i965/brw_blorp.c   |  2 +-
 src/mesa/drivers/dri/i965/brw_nir.c | 18 +-
 src/mesa/drivers/dri/i965/brw_program.c |  4 ++--
 src/mesa/state_tracker/st_glsl_to_nir.cpp   |  2 +-
 7 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index ac11998..e996e0e 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -2324,7 +2324,8 @@ void nir_assign_var_locations(struct exec_list *var_list, 
unsigned *size,
 
 void nir_lower_io(nir_shader *shader,
   nir_variable_mode modes,
-  int (*type_size)(const struct glsl_type *));
+  int (*type_size)(const struct glsl_type *),
+  bool use_load_interpolated_input_intrinsics);
 nir_src *nir_get_io_offset_src(nir_intrinsic_instr *instr);
 nir_src *nir_get_io_vertex_index_src(nir_intrinsic_instr *instr);
 
diff --git a/src/compiler/nir/nir_lower_io.c b/src/compiler/nir/nir_lower_io.c
index b05a73f..aa8a517 100644
--- a/src/compiler/nir/nir_lower_io.c
+++ b/src/compiler/nir/nir_lower_io.c
@@ -39,6 +39,7 @@ struct lower_io_state {
void *mem_ctx;
int (*type_size)(const struct glsl_type *type);
nir_variable_mode modes;
+   bool use_interpolated_input;
 };
 
 void
@@ -394,7 +395,8 @@ nir_lower_io_block(nir_block *block,
 static void
 nir_lower_io_impl(nir_function_impl *impl,
   nir_variable_mode modes,
-  int (*type_size)(const struct glsl_type *))
+  int (*type_size)(const struct glsl_type *),
+  bool use_interpolated_input)
 {
struct lower_io_state state;
 
@@ -402,6 +404,7 @@ nir_lower_io_impl(nir_function_impl *impl,
state.mem_ctx = ralloc_parent(impl);
state.modes = modes;
state.type_size = type_size;
+   state.use_interpolated_input = use_interpolated_input;
 
nir_foreach_block(block, impl) {
   nir_lower_io_block(block, );
@@ -413,11 +416,15 @@ nir_lower_io_impl(nir_function_impl *impl,
 
 void
 nir_lower_io(nir_shader *shader, nir_variable_mode modes,
- int (*type_size)(const struct glsl_type *))
+ int (*type_size)(const struct glsl_type *),
+ bool use_interpolated_input)
 {
nir_foreach_function(function, shader) {
-  if (function->impl)
- nir_lower_io_impl(function->impl, modes, type_size);
+  if (function->impl) {
+ nir_lower_io_impl(function->impl, modes, type_size,
+   use_interpolated_input &&
+   shader->stage == MESA_SHADER_FRAGMENT);
+  }
}
 }
 
diff --git a/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c 
b/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
index 41532fc..a8a8c1b 100644
--- a/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
+++ b/src/gallium/drivers/freedreno/ir3/ir3_cmdline.c
@@ -93,7 +93,7 @@ load_glsl(unsigned num_files, char* const* files, 
gl_shader_stage stage)
// TODO nir_assign_var_locations??
 
NIR_PASS_V(nir, nir_lower_system_values);
-   NIR_PASS_V(nir, nir_lower_io, nir_var_all, st_glsl_type_size);
+   NIR_PASS_V(nir, nir_lower_io, nir_var_all, st_glsl_type_size, false);
NIR_PASS_V(nir, nir_lower_samplers, prog);
 
return nir;
diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c 
b/src/mesa/drivers/dri/i965/brw_blorp.c
index 282a5b2..0473cfe 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.c
+++ b/src/mesa/drivers/dri/i965/brw_blorp.c
@@ -209,7 +209,7 @@ brw_blorp_compile_nir_shader(struct brw_context *brw, 
struct nir_shader *nir,
   unsigned end = var->data.location + nir_uniform_type_size(var->type);
   nir->num_uniforms = MAX2(nir->num_uniforms, end);
}
-   nir_lower_io(nir, nir_var_uniform, nir_uniform_type_size);
+   nir_lower_io(nir, nir_var_uniform, nir_uniform_type_size, false);
 
const unsigned *program =
   brw_compile_fs(compiler, brw, mem_ctx, wm_key, _prog_data, nir,
diff --git a/src/mesa/drivers/dri/i965/brw_nir.c 
b/src/mesa/drivers/dri/i965/brw_nir.c
index 6c3e1d1..caf9fe0 100644
--- a/src/mesa/drivers/dri/i965/brw_nir.c
+++ b/src/mesa/drivers/dri/i965/brw_nir.c
@@ -204,7 +204,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
 * loaded as one vec4 or dvec4 per element (or matrix column), depending on
 * whether it is a double-precision type or not.
 */
-   nir_lower_io(nir, nir_var_shader_in, type_size_vs_input);
+   nir_lower_io(nir,