[Mesa-dev] [PATCH 4/4] i965: Silence unused parameter warnings

Eric Engestrom eric at engestrom.ch
Sun Oct 16 20:51:02 UTC 2016


On Friday, 2016-10-14 19:26:38 -0700, Ian Romanick wrote:
> On 10/14/2016 05:44 PM, Eric Engestrom wrote:
> >> Subject: [PATCH 4/4] i965: Silence unused parameter warnings
> > 
> > How about "remove unused parameters" instead?
> > Silencing the warnings is nothing more than a side effect of this
> > change, albeit the reason you realised it was needed.
> 
> There are already quite a few commits in Mesa with subjects that match
> this pattern.  I've generally used this when the purpose of the change
> is to make the compiler complain less.
> 
> Also... not all of the changes in this commit remove the unused
> parameter. :)

Fair enough :)

> 
> > (Sorry, I've seen so many "silence warning" commits at $DAYJOB that
> > this has become a pet peeve of mine)
> > 
> > One suggestion below, but the series looks good:
> > Reviewed-by: Eric Engestrom <eric at engestrom.ch>
> > 
> > 
> > On Fri, Oct 14, 2016 at 11:59:47AM -0700, Ian Romanick wrote:
> >> From: Ian Romanick <ian.d.romanick at intel.com>
> >>
> >> brw_link.cpp:76:44: warning: unused parameter ‘shader_type’ [-Wunused-parameter]
> >>                             gl_shader_stage shader_type,
> >>                                             ^
> >> brw_nir.c: In function ‘brw_nir_lower_vs_inputs’:
> >> brw_nir.c:194:55: warning: unused parameter ‘devinfo’ [-Wunused-parameter]
> >>                          const struct gen_device_info *devinfo,
> >>                                                        ^
> >> brw_vec4_visitor.cpp:914:37: warning: unused parameter ‘sampler’ [-Wunused-parameter]
> >>                             uint32_t sampler,
> >>                                      ^
> >> brw_vec4_visitor.cpp:1146:34: warning: unused parameter ‘stream_id’ [-Wunused-parameter]
> >>  vec4_visitor::gs_emit_vertex(int stream_id)
> >>                                   ^
> >>
> >> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_link.cpp         | 3 +--
> >>  src/mesa/drivers/dri/i965/brw_nir.c            | 1 -
> >>  src/mesa/drivers/dri/i965/brw_nir.h            | 1 -
> >>  src/mesa/drivers/dri/i965/brw_vec4.cpp         | 2 +-
> >>  src/mesa/drivers/dri/i965/brw_vec4.h           | 2 +-
> >>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp     | 2 +-
> >>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 3 +--
> >>  7 files changed, 5 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp
> >> index 02151d6..5ea9773 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> >> @@ -73,7 +73,6 @@ brw_shader_precompile(struct gl_context *ctx,
> >>  
> >>  static void
> >>  brw_lower_packing_builtins(struct brw_context *brw,
> >> -                           gl_shader_stage shader_type,
> >>                             exec_list *ir)
> >>  {
> >>     /* Gens < 7 don't have instructions to convert to or from half-precision,
> >> @@ -105,7 +104,7 @@ process_glsl_ir(struct brw_context *brw,
> >>     /* lower_packing_builtins() inserts arithmetic instructions, so it
> >>      * must precede lower_instructions().
> >>      */
> >> -   brw_lower_packing_builtins(brw, shader->Stage, shader->ir);
> >> +   brw_lower_packing_builtins(brw, shader->ir);
> >>     do_mat_op_to_vec(shader->ir);
> >>  
> >>     unsigned instructions_to_lower = (DIV_TO_MUL_RCP |
> >> diff --git a/src/mesa/drivers/dri/i965/brw_nir.c b/src/mesa/drivers/dri/i965/brw_nir.c
> >> index 744865b..a935f42 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_nir.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_nir.c
> >> @@ -191,7 +191,6 @@ remap_patch_urb_offsets(nir_block *block, nir_builder *b,
> >>  
> >>  void
> >>  brw_nir_lower_vs_inputs(nir_shader *nir,
> >> -                        const struct gen_device_info *devinfo,
> >>                          bool is_scalar,
> >>                          bool use_legacy_snorm_formula,
> >>                          const uint8_t *vs_attrib_wa_flags)
> >> diff --git a/src/mesa/drivers/dri/i965/brw_nir.h b/src/mesa/drivers/dri/i965/brw_nir.h
> >> index 425d6ce..aef5c53 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_nir.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_nir.h
> >> @@ -99,7 +99,6 @@ nir_shader *brw_preprocess_nir(const struct brw_compiler *compiler,
> >>  bool brw_nir_lower_intrinsics(nir_shader *nir,
> >>                                struct brw_stage_prog_data *prog_data);
> >>  void brw_nir_lower_vs_inputs(nir_shader *nir,
> >> -                             const struct gen_device_info *devinfo,
> >>                               bool is_scalar,
> >>                               bool use_legacy_snorm_formula,
> >>                               const uint8_t *vs_attrib_wa_flags);
> >> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> index 6aa9102..362f32b 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> >> @@ -2114,7 +2114,7 @@ brw_compile_vs(const struct brw_compiler *compiler, void *log_data,
> >>     nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> >>     shader = brw_nir_apply_sampler_key(shader, compiler->devinfo, &key->tex,
> >>                                        is_scalar);
> >> -   brw_nir_lower_vs_inputs(shader, compiler->devinfo, is_scalar,
> >> +   brw_nir_lower_vs_inputs(shader, is_scalar,
> >>                             use_legacy_snorm_formula, key->gl_attrib_wa_flags);
> >>     brw_nir_lower_vue_outputs(shader, is_scalar);
> >>     shader = brw_postprocess_nir(shader, compiler->devinfo, is_scalar);
> >> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> >> index 1505ba6..62c6007 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> >> @@ -262,7 +262,7 @@ public:
> >>                       src_reg offset_value,
> >>                       src_reg mcs,
> >>                       uint32_t surface, src_reg surface_reg,
> >> -                     uint32_t sampler, src_reg sampler_reg);
> >> +                     src_reg sampler_reg);
> >>  
> >>     src_reg emit_mcs_fetch(const glsl_type *coordinate_type, src_reg coordinate,
> >>                            src_reg surface);
> >> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> >> index 1d834a4..7b36fca 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> >> @@ -1967,7 +1967,7 @@ vec4_visitor::nir_emit_texture(nir_tex_instr *instr)
> >>                  shadow_comparitor,
> >>                  lod, lod2, sample_index,
> >>                  constant_offset, offset_value, mcs,
> >> -                texture, texture_reg, sampler, sampler_reg);
> >> +                texture, texture_reg, sampler_reg);
> >>  }
> >>  
> >>  void
> >> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> >> index 3e785bc..eca753c 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> >> @@ -909,7 +909,6 @@ vec4_visitor::emit_texture(ir_texture_opcode op,
> >>                             src_reg mcs,
> >>                             uint32_t surface,
> >>                             src_reg surface_reg,
> >> -                           uint32_t sampler,
> >>                             src_reg sampler_reg)
> >>  {
> >>     /* The sampler can only meaningfully compute LOD for fragment shader
> >> @@ -1141,7 +1140,7 @@ vec4_visitor::emit_gen6_gather_wa(uint8_t wa, dst_reg dst)
> >>  }
> >>  
> >>  void
> >> -vec4_visitor::gs_emit_vertex(int stream_id)
> >> +vec4_visitor::gs_emit_vertex(int /* stream_id */)
> > 
> > `UNUSED`?
> 
> I generally prefer this method in C++ code.  With UNUSED you don't get
> any notification when you start to use a previously unused parameter.
> We previously used (void) casting, and that had the same problem.  If
> the parameter has no name, you know, without a doubt, when you start
> using a parameter that was previously unused.

That's actually a very good point, I never thought about that. I might be
about to make a few changes to our codebase at $DAYJOB following your method.
Thanks!

> 
> It also makes it easier to scan all the method implementations and
> determine that the parameter is never used in any derived class.  Right
> now I can do this:
> 
> $ grep -r gs_emit_vertex src/mesa/drivers/dri/
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp:vec4_visitor::gs_emit_vertex(int /* stream_id */)
> src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp:vec4_gs_visitor::gs_emit_vertex(int stream_id)
> src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp:      gs_emit_vertex(stream_id);
> src/mesa/drivers/dri/i965/gen6_gs_visitor.h:   virtual void gs_emit_vertex(int stream_id);
> src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp:gen6_gs_visitor::gs_emit_vertex(int stream_id)
> src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h:   virtual void gs_emit_vertex(int stream_id);
> src/mesa/drivers/dri/i965/brw_vec4.h:   virtual void gs_emit_vertex(int stream_id);
> 
> Assuming that everyone uses the same technique, I see that stream_id is
> almost always used.  I don't even have to look at the implementations.
> If they were marked UNUSED, it might be incorrect.  Someone may have
> started using the parameter without removing UNUSED.  Ditto for (void)
> casting.

Nice trick!

> 
> >>  {
> >>     unreachable("not reached");
> >>  }
> >> -- 
> >> 2.5.5
> > 
> 


More information about the mesa-dev mailing list