<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 10, 2018 at 8:17 AM, Rob Clark <<a href="mailto:robdclark@gmail.com" target="_blank">robdclark@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Apr 10, 2018 at 11:04 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > On Tue, Apr 10, 2018 at 6:20 AM, Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote: >> >> On Mon, Apr 9, 2018 at 10:52 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> >> wrote: >> > + A bunch of potentially interested parties. >> > >> > On Mon, Apr 9, 2018 at 4:25 PM, Caio Marcelo de Oliveira Filho >> > <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> 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? > </div></div>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. </blockquote><div> </div><div>That would be worth investigating. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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??) </blockquote> </div><div class="gmail_quote">Having a shim seems like a reasonable plan. </div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >> >> 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 </blockquote></div> </div><div class="gmail_extra">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. </div></div>