<div dir="ltr">Rather lively discussion we've got going here...<br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 8, 2018 at 3:23 PM, 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">On Sun, Apr 8, 2018 at 5:54 PM, Bas Nieuwenhuizen<br>
<div><div class="h5"><<a href="mailto:bas@basnieuwenhuizen.nl">bas@basnieuwenhuizen.nl</a>> wrote:<br>
> On Sun, Apr 8, 2018 at 11:40 PM, Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote:<br>
>> On Sun, Apr 8, 2018 at 5:20 PM, Bas Nieuwenhuizen<br>
>> <<a href="mailto:bas@basnieuwenhuizen.nl">bas@basnieuwenhuizen.nl</a>> wrote:<br>
>>>>>>>>>>> +<br>
>>>>>>>>>>> +   /** The mode of the underlying variable */<br>
>>>>>>>>>>> +   nir_variable_mode mode;<br>
>>>>>>>>>><br>
>>>>>>>>>> In fact, it seems like deref->mode is unused outside of nir_print and<br>
>>>>>>>>>> nir_validate.. for logical addressing we can get the mode from the<br>
>>>>>>>>>> deref_var->var at the start of the chain, and deref->mode has no<br>
>>>>>>>>>> meaning for physical addressing (where the mode comes from the<br>
>>>>>>>>>> pointer).<br>
>>>>>>>>>><br>
>>>>>>>>>> So maybe just drop deref->mode?<br>
>>>>>>>>><br>
>>>>>>>>> Isn't it still useful with logical addressing in case a var is not<br>
>>>>>>>>> immediately available? (think VK_KHR_variable_pointers)<br></div></div></blockquote><div><br></div><div>Yes, the idea here is to basically be the NIR equivalent of storage classes.  For cases where you can chase it all the way back to the variable, this is pointless.  For cases where you can't, this can be very useful.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>>>>>>>> not sure, maybe this should just also use fat-pointers like physical<br>
>>>>>>>> addressing does??<br></div></div></blockquote><div> <br></div><div>I *think* (without knowing the future with perfect accuracy) that I'd like us to start moving away from fat pointers in SPIR-V -> NIR.  They've been working ok thus far but we already have had complaints from the radv guys that they don't want fat pointers for SLM and I'm not sure there that great of a fit for other things.  The big reason is that it's actually fairly painful to change from one fat pointer scheme to another if whatever the spirv_to_nir authors picked doesn't really fit your hardware.<br><br></div><div>I'm not 100% sure what to do but the idea I've got at the moment is something like this:<br><br></div><div> 1) For every mode (or storage class), the driver provides to spirv_to_nir a glsl_type which is the type it wants the pointer represented as.<br></div><div> 2) Add a deref_to_pointer intrinsic to convert the deref to that type and use casts to convert the fat pointer to a deref again<br><br></div><div>Why the deref_to_pointer intrinsic?  Because right now some annoying details about nir_intrinsic_info force us to have all drefs have a single component.  If we didn't, then nir_intrinsic_store_var would have two variable-size sources which aren't the same size.  We could modify the rules and have a concept of a "deref source" and to nir_intrinsic_info and say that a deref source can be any size.  That would also work and may actually be a better plan.  I'm open to other suggestions as well.<br><br></div><div>The key point, however, is (1) where we just let the driver choose the storage type of pointers and then they can have whatever lowering pass they want.  If that's a "standard" fat-pointers pass in nir_lower_io, so be it.  If they want to translate directly into LLVM derefs, they can do that too, I suppose.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>>>>>>>>> Also I could see this being useful in physical addressing too to avoid<br>
>>>>>>>>> all passes working with derefs needing to do the constant folding?<br></div></div></blockquote><div><br></div><div>Constant folding is cheap, so I'm not too worried about that.  The bigger thing that actual derefs gain us is the ability of the compiler to reason about derefs.  Suppose, for instance, that you had the following:<br><br></div><div>struct S {<br></div><div>   float a;<br></div><div>   float b;<br>};<br><br></div><div>location(set = 0, binding = 0) Block {<br></div><div>   S arr[10];<br></div><div>} foo;<br><br></div><div>void<br></div><div>my_func(int i)<br>{<br></div><div>   foo.arr[i].a = make_a_value();<br></div><div>   foo.arr[i].b = make_another_value();<br></div><div>   use_value(foo.arr[i].a);<br></div><div>}<br><br></div><div>If everything is lowered to fat pointers, the compiler can't very easily tell that the second line of my_func() didn't stomp foo.arr[i].a and so it has to re-load the value in the third line.  With derefs, we can easily see that the second line didn't stomp it and just re-use the re-use the result of the make_a_value() call.  Yes, you may be able to tell the difference between foo.arr[i].a and foo.arr[i].b with some fancy analysis and arithmetic tracking but it'd be very painful to do.  I've given this substantial thought for almost two years now and haven't actually come up with a good way to do it without the information provided by derefs.<br><br></div><div>The biggest thing that I think we will gain from deref instructions isn't fancy syntax sugar and better nir_printability of pointers.  It also isn't the removal of explicit of fat pointers in spirv_to_nir (thought that is also nice).  The biggest thing I'm going for is improving NIR's ability to optimize UBO, SSBO, and SLM access.  We're pretty good today for local variables (though there are a couple of known places for improvements) but we're utterly rubbish for anything that leaves the current shader invocation.  If we want competent compute performance (which we do!) we need to solve that problem.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>>>>>>>> The problem is that you don't necessarily know the type at compile<br>
>>>>>>>> time (and in the case where you do, you need to do constant folding to<br>
>>>>>>>> figure it out)<br>
>>>>>>><br>
>>>>>>> So I have two considerations here<br>
>>>>>>><br>
>>>>>>> 1) for vulkan you always know the mode, even when you don't know the var.<br>
>>>>>>> 2)  In CL the mode can still get annotated in the source program (CL C<br>
>>>>>>> non-generic pointers) in cases in which we cannot reasonably figure it<br>
>>>>>>> out with just constant folding. In those cases the mode is extra<br>
>>>>>>> information that you really lose.<br>
>>>>>><br>
>>>>>> so, even in cl 1.x, you could do things like 'somefxn(foo ? global_ptr<br>
>>>>>> : local_ptr)'.. depending on how much we inline all the things, that<br>
>>>>>> might not get CF'd away.<br>
>>><br>
>>> How does this even work btw? somefxn has a definition, and the<br>
>>> definition specifies a mode for the argument right? (which is<br>
>>> implicitly __private if the app does not specify anything?)<br>
>><br>
>> iirc, the cl spec has an example something along these lines..<br>
>><br>
>> it doesn't require *physical* storage for anything where you don't<br>
>> know what the ptr type is, however.. so fat ptrs in ssa space works<br>
>> out<br>
>><br>
>>>>><br>
>>>>> But something like<br>
>>>>> __constant int *ptr_value = ...;<br>
>>>>> store ptr in complex data structure.<br>
>>>>> __constant int* ptr2 = load from complex data structure.<br>
>>>>><br>
>>>>> Without explicitly annotating ptr2 it is unlikely that constant<br>
>>>>> folding would find that ptr2 is pointing to __constant address space.<br>
>>>>> Hence removing the modes loses valuable information that you cannot<br>
>>>>> get back by constant folding. However, if you have a pointer with<br>
>>>>> unknown mode, we could have a special mode (or mode_all?) and you can<br>
>>>>> use the uvec2 representation in that case?<br></div></div></blockquote><div><br></div><div>I've thought about using a mode of 0 for this or maybe letting you OR all the possible modes together since nir_variable_mode is, after all, a bitfield.  I don't have any huge preference at the moment.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>>>> hmm, I'm not really getting how deref->mode could magically have<br>
>>>> information that fatptr.y doesn't have.. if the mode is known, vtn<br>
>>>> could stash it in fatptr.y and everyone is happy?  If vtn doesn't know<br>
>>>> this, then I don't see how deref->mode helps..<br>
>>><br>
>>> You mean insert it into the fatptr every time deref_cast is called?<br>
>>><br>
>>> Wouldn't that blow up the IR size significantly for very little benefit?<br>
>><br>
>> in an easy to clean up way, so meh?<br>
><br>
> We can't clean it up if we want to keep the information. Also nir is<br>
> pretty slow to compile already, so I'd like not to add a significant<br>
> number of instruction for very little benefit.<br></div></div></blockquote><div><br></div><div>Really?  I mean, I can believe it, but do you have any actual numbers to back this up?  It's considerably faster than some other IRs we have in mesa though they are known to be pretty big pigs if we're honest.  I'm very open to any suggestions on how to improve compile times.  If NIR is fundamentally a pig, we should fix that.<br><br>I don't think compile time should be the deciding factor in whether or not we use fat pointers as I doubt it will have a huge effect.  However, how much work it takes to get at information is a reasonable argument.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
</div></div>I guess I'm failing to see what information you'd loose.. or how there<br>
would be a problem..<br>
<br>
I'm not strictly against having nir_var_unknown as a possible value<br>
for deref->mode, but I'm not really seeing how that helps anything,<br>
other than adding extra complexity to the IR..<br>
<div><div class="h5"><br>
<br>
>>>>>><br>
>>>>>> I think I'm leaning towards using fat ptrs for the vk case, since I<br>
>>>>>> guess that is a case where you could always expect<br>
>>>>>> nir_src_as_const_value() to work, to get the variable mode.  If for no<br>
>>>>>> other reason than I guess these deref's, if the var is not known,<br>
>>>>>> start w/ deref_cast, and it would be ugly for deref_cast to have to<br>
>>>>>> work differently for compute vs vk.  But maybe Jason already has some<br>
>>>>>> thoughts about it?<br></div></div></blockquote><div><br></div><div>I don't see why they would be different.  Vulkan and CL are starting to converge.  In vulkan, you already can't always chase the pointer back to the variable VK_KHR_variable_pointers is used.  I don't think that CL is as special as you think it is.  The only thing special about CL is the fact that global and local are in the same address space.  Beyond that, all the problems we have to solve are fundamentally the same.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>>>>> I'd like to avoid fat pointers alltogether on AMD since we would not<br>
>>>>> use it even for CL. a generic pointer is just a uint64_t for us, with<br>
>>>>> no bitfield in there for the address space.<br></div></div></blockquote><div><br></div><div>I tend to agree with this.  (See above discussion).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>>>>> I think we may need to think a bit more about representation however,<br>
>>>>> as e.g. for AMD a pointer is typically 64-bits (but we can do e.g.<br>
>>>>> 32-bits for known workgroup pointers), the current deref instructions<br>
>>>>> return 32-bit, and you want something like a uvec2 as pointer<br>
>>>>> representation?<br>
>>>><br>
>>>> afaiu, newer AMD (and NV) hw can remap shared/private into a single<br>
>>>> global address space..  But I guess that is an easy subset of the<br>
>>>> harder case where drivers need to use different instructions.. so a<br>
>>>> pretty simple lowering pass run before lower_io could remap things<br>
>>>> that use fatptrs into something that ignores fatptr.y.  Then opt<br>
>>>> passes make fatptr.y go away.  So both AMD and hw that doesn't have a<br>
>>>> flat address space are happy.<br>
>>><br>
>>> But then you run into other issues, like how are you going to stuff a<br>
>>> 64-bit fatptr.x + a ?-bit fatptr.y into a 64-bit value for Physical64<br>
>>> addressing? Also this means we have to track to the sources back to<br>
>>> the cast/var any time we want to do anything at all with any deref<br>
>>> which seems less efficient to me than just stuffing the deref in<br>
>>> there.<br>
>><br>
>> so fat ptrs only have to exist in ssa space, not be stored to<br>
>> something with a physically defined size..<br>
><br>
> how does storing __generic pointers work then? those still need the<br>
> fat bit for your hw right?<br>
<br>
</div></div>for hw that can't map everything into a single flat address space, you<br>
*can't* store a fat pointer.. oh well, it doesn't mean that that hw<br>
can't implement lesser ocl versions (since this is defn not required<br>
for ocl 1.x and I don't *think* it is required for 2.x, but I haven't<br>
checked the spec.. I guess if intel supports 2.x then it isn't<br>
required..)<span class=""><br></span></blockquote><div><br></div><div>You can still carve up the address space and put local memory at some absurdly high address and do<br><br></div><div>if (ptr > 0xffffffffffff0000)<br></div><div>   store_local(ptr & 0xffff)<br></div><div>else<br></div><div>   store_global(ptr)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>><br>
>> As far as tracking things to the head of the chain of deref<br>
>> instructions, that is a matter of a simple helper or two.  Not like<br>
>> the chain of deref's is going to be 1000's of instructions..<br></span></blockquote><div><br></div><div>Yes and no (mostly no) once you start throwing in phis, selects, and loading pointers from variables.  The first two you can explain away by just saying, "follow the phi/select sources" but the moment you load a pointer out of a variable, you have to do something.  Sure, you could just drop .y and stomp it to the known storage class, but that hardly seems ideal.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>>> Also, what would the something which ignores fatptr.y be? I'd assume<br>
>>> that would be the normal deref based stuff, but requiring fatptr<br>
>>> contradicts that?<br>
>><br>
>> if you have a flat address space, maybe a pass (or option for<br>
>> lower_io) to just convert everything to load/store_global (since<br>
>> essentially what these GPUs are doing is remapping shared/private into<br>
>> the global address space)<br>
><br>
> but I'd like to only do that if we really don't know the mode<br>
> statically since it is somewhat slower/less flexible. (also radv is<br>
> unlikely to use nir_lower_io for a lot of this stuff since we can<br>
> lower derefs into LLVM GEPs directly)<br>
<br>
</span>I mean, we control all the src code, we can add more options to lower_io.<br>
<span class=""><br>
><br>
> Hence if we want the cases where we know the mode statically we need<br>
> to not lower the fatptr, but then we have the whole fatptr mess.<br>
><br>
<br>
</span>well, fatptr mess is unavoidable.. I mostly just fail to see how a<br>
more general solution (ie. storing the variable mode in ssa) is worse<br>
than having to deal with both cases (in ssa or in instr).  The in ssa<br>
case is something that is easy to recover since anything that does<br>
pointer math has to split out the .y component and fuse it back in<br>
after the pointer math.  (And if you have a flat address space, I<br>
guess I'm failing to see why you care about the mode in the first<br>
place.)<br>
</blockquote></div><br></div><div class="gmail_extra">Yes, fat pointers are inevitable for some hardware at some stage in the compile.  That does not, however, mean that we want fat pointers straight out of SPIR-V.<br></div></div>