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

[Rob Clark]

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).  Although I'd kinda like
to think that we don't need to make vtn aware of this distinction.

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.

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)

BR,
-R


> Thoughts?
>
> --Jason