[Mesa-dev] [PATCH v3 057/104] nir,spirv: Rework function calls

[Jason Ekstrand]

On Tue, Apr 10, 2018 at 6:20 AM, Rob Clark <robdclark at gmail.com> wrote:

> On Mon, Apr 9, 2018 at 10:52 PM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> > + A bunch of potentially interested parties.
> >
> > On Mon, Apr 9, 2018 at 4:25 PM, Caio Marcelo de Oliveira Filho
> > <caio.oliveira at intel.com> wrote:
> >>
> >> Hi,
> >>
> >> >  typedef struct {
> >> > -   nir_parameter_type param_type;
> >> > -   const struct glsl_type *type;
> >> > +   uint8_t num_components;
> >> > +   uint8_t bit_size;
> >> >  } nir_parameter;
> >>
> >> (...)
> >>
> >> > @@ -683,18 +692,12 @@ validate_tex_instr(nir_tex_instr *instr,
> >> > validate_state *state)
> >> >  static void
> >> >  validate_call_instr(nir_call_instr *instr, validate_state *state)
> >> >  {
> >> > -   if (instr->return_deref == NULL) {
> >> > -      validate_assert(state,
> >> > glsl_type_is_void(instr->callee->return_type));
> >> > -   } else {
> >> > -      validate_assert(state, instr->return_deref->deref.type ==
> >> > instr->callee->return_type);
> >> > -      validate_deref_var(instr, instr->return_deref, state);
> >> > -   }
> >> > -
> >> >     validate_assert(state, instr->num_params ==
> >> > instr->callee->num_params);
> >> >
> >> >     for (unsigned i = 0; i < instr->num_params; i++) {
> >> > -      validate_assert(state, instr->callee->params[i].type ==
> >> > instr->params[i]->deref.type);
> >> > -      validate_deref_var(instr, instr->params[i], state);
> >> > +      validate_src(&instr->params[i], state,
> >> > +                   instr->callee->params[i].bit_size,
> >> > +                   instr->callee->params[i].num_components);
> >> >     }
> >> >  }
> >>
> >> Question: I might be misreading, but it seems like we are losing the
> >> type information for functions. Isn't that something worth keeping,
> >> maybe in some other way, e.g. load_param specifying the expected type?
> >
> >
> > That's a very good question!  To be honest, I'm not sure what the answer
> is.
> > At the moment, the type information is fairly useless for most of what we
> > use functions for.  Really, all we need is something that NIR can inline.
> > As it is, we're not really preserving the types from SPIR-V because of
> the
> > gymnastics we're doing to handle pointers.
> >
> > If we did want to preserve types, we'd need to have more detailed type
> > information.  In particular, we'd need to be able to provide pointer
> types
> > and maybe combined image-sampler types.  And along with those pointer
> types,
> > we'd need to somehow express those pointer's storage requirements.
> >
> > The philosophy behind this commit is that, if we don't have a good match
> to
> > SPIR-V anyway, we might as well just chuck that information and do
> whatever
> > makes our lives the easiest.  My philosophy here may be flawed and I'm
> happy
> > to hear arguments in favor of keeping the information.  The best
> argument I
> > can come up with for keeping the information is if we find ourselves
> wanting
> > to do some sort of linking in the future where we have to match
> functions by
> > both name and type.  If we want to do that, however, we'll need all the
> > SPIR-V type information.
> >
>
> We do end up wanting the type information for cl kernels.  This is
> maybe a slightly different case from calls within shader code (ie.
> when both caller and callee are in shader).


Yes, I think it is.  Question: Is there a distinction in CL between
functions which are entrypoints callable from the API and functions which
are helpers?  i.e. Can you call an entrypoint as a helper?


> Although I'd kinda like
> to think that we don't need to make vtn aware of this distinction.
>

Someone has to be aware of it. :-)  There are lots of places in
spirv_to_nir were we take the SPIR-V and do something slightly different
with it than the obvious translation.  Also, using function parameters for
this is a significant anachronism because no other shader I/O in NIR has
ever worked that way.


> So just to throw out an idea.  What if vtn just used load_deref for
> everything, and in the case of fxn params it just points to a local
> var with type nir_var_param?  (Or something roughly like that.)  Then
> lower_io lowers this to load_param.
>

That's kind-of what the original thing did.  However, for SPIR-V helper
functions we have to be able to pass through pointers, SSA values with
arbitrary type, and image/sampler pointers.  SSA values can be handled by
just making a variable and storing them to it.  Pointers are tricky because
they're not really copy-in/out.  For images, samplers, and pointers, we
have a pile of "try to patch up the deref chain" code in
nir_inline_functions that's rather tricky.  The moral of the story is that
"just use variables" is not nearly as obvious of a choice as it looks.


> This way clover could use it's own pass to lower kernel entrypoint
> load_deref's to load_param differently (ie. the offset becomes byte
> offset into input buffer instead of idx)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180410/ea23c677/attachment.html>