[PATCH vkd3d 4/5] vkd3d-shader: Return the register space of each used UAV and UAV counter in vkd3d_shader_scan_info.

[Zebediah Figura]

On 6/4/20 5:04 PM, Henri Verbeet wrote:
> On Fri, 5 Jun 2020 at 01:39, Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> On 6/4/20 3:48 PM, Henri Verbeet wrote:
>>> On Thu, 4 Jun 2020 at 06:20, Zebediah Figura <zfigura at codeweavers.com> wrote:
>>>> +struct vkd3d_uav_scan_info
>>>> +{
>>>> +    unsigned int register_space, register_index, id;
>>>> +    bool counter, read;
>>>> +};
>>>> +
>>> It seems tempting to store the "counter" and "read" fields as a
>>> "flags" field instead. For what it's worth, note that I also have
>>> patches in this area in my own tree; we'll need to store information
>>> about the other descriptor types in the vkd3d_shader_scan_info at some
>>> point as well.
>>
>> Sure, that makes sense.
>>
>>> I'm not sure whether there's a use for the "id" field in the public
>>> API. I'm inclined to say lookup by ID should go through the
>>> "symbol_table" tree.
>>
>> Yes, I don't see a reason to expose "id".
>>
>> Is the idea then to introduce a new symbol table in struct
>> vkd3d_shader_parser? It seems a bit unfortunate that the table would be
>> duplicated, but after all I guess it's not a lot of duplication...
>>
> No, the idea would be that in spirv.c you'd use the symbol table to
> lookup the range information for the ID, and then use that information
> to lookup the counter/read information. E.g.:
> 
> dcl_uav_typed u0[1:3], ..., space=4
> ...
> store_uav_typed u0[1], ...
> 
> The store_uav_typed handler would lookup u0 in the symbol table and
> find that it corresponds to [1:3] in register space 4. Index 1 in that
> range would then be descriptor 2 in that register space 4. It would
> then lookup the information for descriptor 2 in register space 4 in
> the vkd3d_shader_scan_info to check whether that's a readonly UAV.
> (There are of course variants on that, like doing the lookup when
> inserting u0 in the symbol table, etc. To some extent it also depends
> on what we do with the issue below.)

I'm probably misunderstanding something, but how would we find the
register space and index in vkd3d_shader_scan_record_uav_read()?

> 
>>> Perhaps more important is the question how this would work with ranges
>>> larger than a single descriptor. I.e., how would for example a
>>> "dcl_uav_raw u0[1:3], ..." declaration be represented in the
>>> vkd3d_shader_scan_info structure?
>>>
>>
>> It's hard for me to answer that without a full perspective as to how the
>> indices are or might be used, but my best guess is that, if descriptor
>> ranges are uploaded all at once, we'd want to track a single
>> used/counter flag for the whole range.
> 
> It probably needs tests to determine whether it's allowed in practice,
> but the bytecode seems to allow for overlapping ranges. E.g.:
> 
> dcl_uav_raw u0[1:4], ..., space=0
> dcl_uav_raw u1[3:6], ..., space=0
> 
> would make both u0[2] and u1[0] refer to descriptor 3 in register
> space 0. 

In which case I guess we'd mark both ranges as read if descriptor 3 is
read from.

> But even without aliasing, it's not clear to me that e.g.
> using the counter for u0[0] necessarily implies that all of u0 needs
> one.
> 

Yes, certainly, it's more conservative than is probably desirable,
especially as I now examine spirv.c a little closer. Of course, I also
don't know how much of that is redundancy and how much is a performance
concern.

While using e.g. a bitmask may be reasonable, I'm not sure how to
reconcile that with unbounded or even particularly large descriptor
arrays. Perhaps cutting off the bitmask size at a point makes sense (and
treating anything further as "always used"?), but I don't have the
experience to guess what a reasonable cutoff point is.