[Mesa-dev] [PATCH 02/78] i965/nir/vec4: Select between new nir_vec4 or current vec4_visitor code-paths

Mon Jun 29 14:57:45 PDT 2015

On 06/29/2015 10:20 PM, Kenneth Graunke wrote:
> On Friday, June 26, 2015 10:06:18 AM Eduardo Lima Mitev wrote:
>> The NIR->vec4 pass will be activated if ALL the following
>> conditions are met:
>> 
>> * INTEL_USE_NIR environment variable is defined and is positive
>> (1 or true) * The stage is vertex shader * The HW generation is
>> either SandyBridge (gen6), IvyBridge or Haswell (gen7)
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580 --- 
>> src/mesa/drivers/dri/i965/brw_program.c  |  5 +++++ 
>> src/mesa/drivers/dri/i965/brw_shader.cpp | 14 ++++++++------ 
>> src/mesa/drivers/dri/i965/brw_vec4.cpp   | 32
>> ++++++++++++++++++++++++++++++-- 
>> src/mesa/drivers/dri/i965/brw_vec4.h     |  2 ++ 4 files changed,
>> 45 insertions(+), 8 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_program.c
>> b/src/mesa/drivers/dri/i965/brw_program.c index 2327af7..7e5d23d
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_program.c +++
>> b/src/mesa/drivers/dri/i965/brw_program.c @@ -574,6 +574,11 @@
>> brw_dump_ir(const char *stage, struct gl_shader_program
>> *shader_prog, struct gl_shader *shader, struct gl_program *prog) 
>> { if (shader_prog) { +      /* Since git~104c8fc, shader->ir can
>> be NULL if NIR is used. +       * That must have been checked
>> prior to calling this function, but +       * we double-check
>> here just in case. +       */
> 
> That got reverted, so you can probably drop this comment.  The
> assertion seems reasonable.
> 

Ok, I will remove the comment then. There is also a patch from Neil
Roberts in the mailing list
<http://lists.freedesktop.org/archives/mesa-dev/2015-June/087607.html>
that handles this by checking for shader->ir null-ness. Maybe that's
just enough and the assert is not needed.

>> +      assert(shader->ir != NULL); fprintf(stderr, "GLSL IR for
>> native %s shader %d:\n", stage, shader_prog->Name); 
>> _mesa_print_ir(stderr, shader->ir, NULL); diff --git
>> a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> b/src/mesa/drivers/dri/i965/brw_shader.cpp index 5653d6b..0b53647
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_shader.cpp +++
>> b/src/mesa/drivers/dri/i965/brw_shader.cpp @@ -118,12 +118,14 @@
>> brw_compiler_create(void *mem_ctx, const struct brw_device_info
>> *devinfo) 
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].OptimizeForAOS
>> = true; 
>> compiler->glsl_compiler_options[MESA_SHADER_GEOMETRY].OptimizeForAOS
>> = true;
>> 
>> -   if (compiler->scalar_vs) { -      /* If we're using the
>> scalar backend for vertex shaders, we need to -       * configure
>> these accordingly. -       */ -
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].EmitNoIndirectOutput
>> = true; -
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].EmitNoIndirectTemp
>> = true; +   if (compiler->scalar_vs ||
>> brw_env_var_as_boolean("INTEL_USE_NIR", false)) { +      if
>> (compiler->scalar_vs) { +         /* If we're using the scalar
>> backend for vertex shaders, we need to +         * configure
>> these accordingly. +         */
> 
> indentation looks a bit off here.
> 
>> +
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].EmitNoIndirectOutput
>> = true; +
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].EmitNoIndirectTemp
>> = true; +      } 
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].OptimizeForAOS
>> = false;
>> 
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].NirOptions =
>> nir_options; diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> b/src/mesa/drivers/dri/i965/brw_vec4.cpp index a5c686c..dcffa04
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp +++
>> b/src/mesa/drivers/dri/i965/brw_vec4.cpp @@ -1707,6 +1707,21 @@
>> vec4_visitor::emit_shader_time_write(int shader_time_subindex,
>> src_reg value) }
>> 
>> bool +vec4_visitor::should_use_vec4_nir() +{ +   /* NIR->vec4
>> pass is activated when all these conditions meet: +    * +    *
>> 1) it is a vertex shader +    * 2) INTEL_USE_NIR env-var set to
>> true, so NirOptions are defined for VS +    * 3) hardware gen is
>> SNB, IVB or HSW +    */ +   return +      stage ==
>> MESA_SHADER_VERTEX && +
>> compiler->glsl_compiler_options[MESA_SHADER_VERTEX].NirOptions !=
>> NULL && +      devinfo->gen >= 6 && devinfo->gen < 8;
> 
> Why not just do:
> 
> return compiler->glsl_compiler_options[stage].NirOptions != NULL;
> 
> As long as you don't set NirOptions for geometry shaders, that
> will work fine.  You could also only set
> NirOptions[MESA_SHADER_VERTEX] when gen >= 6 && gen < 8, if you
> like.  You could probably just eliminate the function at that
> point.
> 

Yes, you that's right. I'm setting NirOptions only to vertex shaders'
compiler options; and together with Jason suggestion of removing the
gen check, we are left with just a check for NirOptions.

I will change that and remove the method too.

>> +} + +bool vec4_visitor::run(gl_clip_plane *clip_planes) { 
>> sanity_param_count = prog->Parameters->NumParameters; @@ -1722,7
>> +1737,17 @@ vec4_visitor::run(gl_clip_plane *clip_planes) *
>> functions called "main"). */ if (shader) { -
>> visit_instructions(shader->base.ir); +      if
>> (should_use_vec4_nir()) { +         assert(prog->nir != NULL); +
>> emit_nir_code(); +         if (failed) +            return
>> false; +      } else { +         /* Generate VS IR for main().
>> (the visitor only descends into +          * functions called
>> "main"). +          */ +
>> visit_instructions(shader->base.ir); +      } } else { 
>> emit_program_code(); } @@ -1882,7 +1907,10 @@ brw_vs_emit(struct
>> brw_context *brw, st_index = brw_get_shader_time_index(brw, prog,
>> &c->vp->program.Base, ST_VS);
>> 
>> -   if (unlikely(INTEL_DEBUG & DEBUG_VS)) +   /* Since
>> git~104c8fc, shader->base.ir can be NULL if NIR is used, +    *
>> so we need to check that before dumping IR tree. +    */
> 
> That got reverted, so you can probably drop this hunk.
> 
>> +   if (unlikely(INTEL_DEBUG & DEBUG_VS) && (!prog ||
>> shader->base.ir)) brw_dump_ir("vertex", prog, &shader->base,
>> &c->vp->program.Base);
>> 
>> if (brw->intelScreen->compiler->scalar_vs) { diff --git
>> a/src/mesa/drivers/dri/i965/brw_vec4.h
>> b/src/mesa/drivers/dri/i965/brw_vec4.h index 4f2cc86..68bc4ae
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_vec4.h +++
>> b/src/mesa/drivers/dri/i965/brw_vec4.h @@ -389,6 +389,8 @@
>> public:
>> 
>> void visit_atomic_counter_intrinsic(ir_call *ir);
>> 
>> +   virtual bool should_use_vec4_nir(); + virtual void
>> emit_nir_code(); virtual void nir_setup_inputs(nir_shader
>> *shader); virtual void nir_setup_outputs(nir_shader *shader);
>> 