[Mesa-dev] [PATCH 3/5] glsl: fall back to inexact function-match

[Erik Faye-Lund]

On Wed, 2018-10-31 at 12:01 -0400, Ilia Mirkin wrote:
> I had to do a double (or triple) take on this logic as well. Part of
> the subtlety is that the fallback only applies for ES when there's a
> match but no exact match. Probably good to mention this.

Yeah, that makes sense. I thought I mentioneded this in the commit
message, but perhaps you want that to be more explicit than the "In
GLES, "-introduction?

How about I simply add something like "This fallback should only affect
GLES." at the end of the commit message?

> On Wed, Oct 31, 2018 at 11:13 AM Erik Faye-Lund
> <erik.faye-lund at collabora.com> wrote:
> > 
> > On Wed, 2018-10-31 at 15:46 +0200, Tapani Pälli wrote:
> > > 
> > > On 10/30/18 7:11 PM, Erik Faye-Lund wrote:
> > > > In GLES, we currently either need an exact match with a local
> > > > function,
> > > > or an exact match with a builtin.
> > > > 
> > > > However, if we add support for implicit conversions for GLES
> > > > shaders,
> > > > we also need to fall back to a non-exact match in the case
> > > > where
> > > > there
> > > > were no builtin match either.
> > > > 
> > > > Luckily, we already have a variable ready with this, so let's
> > > > just
> > > > return it if the builtin-search failed.
> > > > 
> > > > Signed-off-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
> > > > ---
> > > >   src/compiler/glsl/ast_function.cpp | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/compiler/glsl/ast_function.cpp
> > > > b/src/compiler/glsl/ast_function.cpp
> > > > index 1fa3f7561ae..50fd8bc5f5d 100644
> > > > --- a/src/compiler/glsl/ast_function.cpp
> > > > +++ b/src/compiler/glsl/ast_function.cpp
> > > > @@ -667,7 +667,7 @@ match_function_by_name(const char *name,
> > > >      /* Local shader has no exact candidates; check the built-
> > > > ins.
> > > > */
> > > >      _mesa_glsl_initialize_builtin_functions();
> > > >      sig = _mesa_glsl_find_builtin_function(state, name,
> > > > actual_parameters);
> > > > -   return sig;
> > > > +   return sig ? sig : local_sig;
> > > 
> > > I think this would deserve a comment about 'local_sig', it did
> > > not
> > > seem
> > > obvious ... maybe something like:
> > > 
> > > /* Variable local_sig contains the result from
> > > choose_best_inexact_overload(). */
> > > 
> > 
> > Yeah, good point. I've changed it to this, which feels like it
> > explains
> > the point without spoon-feeding the reader too much... How does
> > that
> > sound?
> > 
> > ---8<---
> > diff --git a/src/compiler/glsl/ast_function.cpp
> > b/src/compiler/glsl/ast_function.cpp
> > index 1fa3f7561ae..6aa30400060 100644
> > --- a/src/compiler/glsl/ast_function.cpp
> > +++ b/src/compiler/glsl/ast_function.cpp
> > @@ -667,7 +667,11 @@ match_function_by_name(const char *name,
> >     /* Local shader has no exact candidates; check the built-ins.
> > */
> >     _mesa_glsl_initialize_builtin_functions();
> >     sig = _mesa_glsl_find_builtin_function(state, name,
> > actual_parameters);
> > -   return sig;
> > +
> > +   /* if _mesa_glsl_find_builtin_function failed, fall back to the
> > result
> > +    * of choose_best_inexact_overload() instead
> > +    */
> > +   return sig ? sig : local_sig;
> >  }
> > 
> >  static ir_function_signature *
> > ---8<---
> > 
> > 
> > > >   }
> > > > 
> > > >   static ir_function_signature *
> > > > 
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev