On Tue, Apr 10, 2018 at 6:20 AM, Rob Clark <robdcl...@gmail.com> wrote:
> On Mon, Apr 9, 2018 at 10:52 PM, Jason Ekstrand <ja...@jlekstrand.net> > wrote: > > + A bunch of potentially interested parties. > > > > On Mon, Apr 9, 2018 at 4:25 PM, Caio Marcelo de Oliveira Filho > > <caio.olive...@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) >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev