From: Zebediah Figura Subject: Re: [PATCH vkd3d 4/5] vkd3d-shader: Return the register space of each used UAV and UAV counter in vkd3d_shader_scan_info. Message-Id: <4095fe3e-4428-d8ec-fae0-2d313164bd48@codeweavers.com> Date: Thu, 4 Jun 2020 17:25:48 -0500 In-Reply-To: References: <20200604014915.986832-1-zfigura@codeweavers.com> <20200604014915.986832-4-zfigura@codeweavers.com> On 6/4/20 5:04 PM, Henri Verbeet wrote: > On Fri, 5 Jun 2020 at 01:39, Zebediah Figura wrote: >> >> On 6/4/20 3:48 PM, Henri Verbeet wrote: >>> On Thu, 4 Jun 2020 at 06:20, Zebediah Figura 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.