[Mesa-dev] [PATCH v3 057/104] nir,spirv: Rework function calls
Jason Ekstrand
jason at jlekstrand.net
Tue Apr 10 15:55:41 UTC 2018
On Tue, Apr 10, 2018 at 8:17 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Tue, Apr 10, 2018 at 11:04 AM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> > 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?
> >
>
> There is the __kernel annotation. And you know the entry point name
> when compiling. However I'm not sure anything prevents one entry
> point from calling another.
>
That would be worth investigating.
> I'm not sure we want the calling convention to be the same internally
> as for kernel entry points so in that case, if we aren't inlining
> everything, we might end up generating two versions of a function (or
> possibly a shim.. or possibly between the two based on size.. or??)
>
Having a shim seems like a reasonable plan.
> >>
> >> 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.
>
> So, I'm considering just adding nir_variable ptr back to
> nir_parameter. That would be rather easy, and gives something that
> some lowering pass could fixup data.driver_loc with parameter
> position. That would be rather easy for driver backends:
>
> param_loc = fxn->params[param_idx]->var->data.driver_loc
>
Why can't you just add a mode nir_var_kernel_param and keep a running
uint32_t offset in the builder. When you process an OpParameter in an
entrypoint, create a nir_var_kernel_param variable and set
var->data.location = offset and add the size of the param to offset. Then
the OpParameter can return an access chain to the newly created interface
variable. I fail to see why load_param is needed at all here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180410/c1799515/attachment-0001.html>
More information about the mesa-dev
mailing list