<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 10, 2018 at 6:20 AM, Rob Clark <span dir="ltr"><<a href="mailto:robdclark@gmail.com" target="_blank">robdclark@gmail.com</a>></span> wrote:<br><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 Mon, Apr 9, 2018 at 10:52 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> + A bunch of potentially interested parties.<br>
><br>
> On Mon, Apr 9, 2018 at 4:25 PM, Caio Marcelo de Oliveira Filho<br>
> <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br>
>><br>
>> Hi,<br>
>><br>
>> > typedef struct {<br>
>> > - nir_parameter_type param_type;<br>
>> > - const struct glsl_type *type;<br>
>> > + uint8_t num_components;<br>
>> > + uint8_t bit_size;<br>
>> > } nir_parameter;<br>
>><br>
>> (...)<br>
>><br>
>> > @@ -683,18 +692,12 @@ validate_tex_instr(nir_tex_<wbr>instr *instr,<br>
>> > validate_state *state)<br>
>> > static void<br>
>> > validate_call_instr(nir_call_<wbr>instr *instr, validate_state *state)<br>
>> > {<br>
>> > - if (instr->return_deref == NULL) {<br>
>> > - validate_assert(state,<br>
>> > glsl_type_is_void(instr-><wbr>callee->return_type));<br>
>> > - } else {<br>
>> > - validate_assert(state, instr->return_deref->deref.<wbr>type ==<br>
>> > instr->callee->return_type);<br>
>> > - validate_deref_var(instr, instr->return_deref, state);<br>
>> > - }<br>
>> > -<br>
>> > validate_assert(state, instr->num_params ==<br>
>> > instr->callee->num_params);<br>
>> ><br>
>> > for (unsigned i = 0; i < instr->num_params; i++) {<br>
>> > - validate_assert(state, instr->callee->params[i].type ==<br>
>> > instr->params[i]->deref.type);<br>
>> > - validate_deref_var(instr, instr->params[i], state);<br>
>> > + validate_src(&instr->params[i]<wbr>, state,<br>
>> > + instr->callee->params[i].bit_<wbr>size,<br>
>> > + instr->callee->params[i].num_<wbr>components);<br>
>> > }<br>
>> > }<br>
>><br>
>> Question: I might be misreading, but it seems like we are losing the<br>
>> type information for functions. Isn't that something worth keeping,<br>
>> maybe in some other way, e.g. load_param specifying the expected type?<br>
><br>
><br>
> That's a very good question! To be honest, I'm not sure what the answer is.<br>
> At the moment, the type information is fairly useless for most of what we<br>
> use functions for. Really, all we need is something that NIR can inline.<br>
> As it is, we're not really preserving the types from SPIR-V because of the<br>
> gymnastics we're doing to handle pointers.<br>
><br>
> If we did want to preserve types, we'd need to have more detailed type<br>
> information. In particular, we'd need to be able to provide pointer types<br>
> and maybe combined image-sampler types. And along with those pointer types,<br>
> we'd need to somehow express those pointer's storage requirements.<br>
><br>
> The philosophy behind this commit is that, if we don't have a good match to<br>
> SPIR-V anyway, we might as well just chuck that information and do whatever<br>
> makes our lives the easiest. My philosophy here may be flawed and I'm happy<br>
> to hear arguments in favor of keeping the information. The best argument I<br>
> can come up with for keeping the information is if we find ourselves wanting<br>
> to do some sort of linking in the future where we have to match functions by<br>
> both name and type. If we want to do that, however, we'll need all the<br>
> SPIR-V type information.<br>
><br>
<br>
</div></div>We do end up wanting the type information for cl kernels. This is<br>
maybe a slightly different case from calls within shader code (ie.<br>
when both caller and callee are in shader).</blockquote><div><br></div><div>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?<br></div><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<br>
to think that we don't need to make vtn aware of this distinction.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So just to throw out an idea. What if vtn just used load_deref for<br>
everything, and in the case of fxn params it just points to a local<br>
var with type nir_var_param? (Or something roughly like that.) Then<br>
lower_io lowers this to load_param.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This way clover could use it's own pass to lower kernel entrypoint<br>
load_deref's to load_param differently (ie. the offset becomes byte<br>
offset into input buffer instead of idx)<br>
</blockquote></div><br></div></div>