On Friday, 2018-11-09 16:15:12 +0200, andrey simiklit wrote:
> Hello,
>
> Thanks for review.
> Please find my comments below:
>
> On Fri, Nov 9, 2018 at 2:52 PM Eric Engestrom <eric.engest...@intel.com>
> wrote:
>
> > On Tuesday, 2018-09-11 15:42:05 +0300, asimiklit.w...@gmail.com wrote:
> > > From: Andrii Simiklit <andrii.simik...@globallogic.com>
> > >
> > > 1. nir/nir_lower_vars_to_ssa.c:691:21: warning:
> > > unused variable ‘var’
> > > nir_variable *var = path->path[0]->var;
> > >
> > > 2. nir_types.cpp:558:31: warning:
> > > ‘elem_align’ may be used uninitialized in this function
> > > unsigned elem_size, elem_align;
> > > nir_types.cpp:558:20: warning:
> > > ‘elem_size’ may be used uninitialized in this function
> > > unsigned elem_size, elem_align;
> > >
> > > Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com>
> > > ---
> > > src/compiler/nir/nir_lower_vars_to_ssa.c | 2 +-
> > > src/compiler/nir_types.cpp | 4 ++--
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/compiler/nir/nir_lower_vars_to_ssa.c
> > b/src/compiler/nir/nir_lower_vars_to_ssa.c
> > > index cd679be..96de261 100644
> > > --- a/src/compiler/nir/nir_lower_vars_to_ssa.c
> > > +++ b/src/compiler/nir/nir_lower_vars_to_ssa.c
> > > @@ -688,7 +688,7 @@ nir_lower_vars_to_ssa_impl(nir_function_impl *impl)
> > > nir_deref_path *path = &node->path;
> > >
> > > assert(path->path[0]->deref_type == nir_deref_type_var);
> > > - nir_variable *var = path->path[0]->var;
> > > + MAYBE_UNUSED nir_variable *var = path->path[0]->var;
> > >
> > > /* We don't build deref nodes for non-local variables */
> > > assert(var->data.mode == nir_var_local);
> >
> > Sure, or you could simply remove the single-use extra local variable and
> > just fold it into the assert :)
> >
>
> I agree,will fix :)
>
>
> >
> > > diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
> > > index d24f094..d39d87b 100644
> > > --- a/src/compiler/nir_types.cpp
> > > +++ b/src/compiler/nir_types.cpp
> > > @@ -542,7 +542,7 @@ glsl_get_natural_size_align_bytes(const struct
> > glsl_type *type,
> > > }
> > >
> > > case GLSL_TYPE_ARRAY: {
> > > - unsigned elem_size, elem_align;
> > > + unsigned elem_size = 0, elem_align = 0;
> >
> > Not really keen on these ones; it looks like your compiler is ignoring the
> > unreachable() in glsl_get_natural_size_align_bytes() and mis-computing the
> > possible code-paths, resulting in it wrongly printing a warning.
> >
>
> You are right when I added the 'default:' case to the switch the warning is
> disappeared:
>
> case GLSL_TYPE_INTERFACE:
> case GLSL_TYPE_FUNCTION:
> +default:
> unreachable("type does not have a natural size");
>
> What do you think about such solution? Is it acceptable for you?
Is there any possible value of `enum glsl_base_type` that is not handled
by the switch? If so, they should be added. Otherwise, this is
a compiler bug, not a code bug :)
>
>
> > I'd prefer to keep these two hunks as is, so that real issue in
> > glsl_get_natural_size_align_bytes() would get flagged appropriately.
> >
> > > glsl_get_natural_size_align_bytes(type->fields.array,
> > > &elem_size, &elem_align);
> > > *align = elem_align;
> > > @@ -554,7 +554,7 @@ glsl_get_natural_size_align_bytes(const struct
> > glsl_type *type,
> > > *size = 0;
> > > *align = 0;
> > > for (unsigned i = 0; i < type->length; i++) {
> > > - unsigned elem_size, elem_align;
> > > + unsigned elem_size = 0, elem_align = 0;
> > >
> > glsl_get_natural_size_align_bytes(type->fields.structure[i].type,
> > > &elem_size, &elem_align);
> > > *align = MAX2(*align, elem_align);
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
