Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Broaden resources support v2, part 3.

Merged Francisco Casas requested to merge fcasas/vkd3d:master6_part_3 into master

Mainly promoting single object components to variables for SM 5.0's RDEF block and lowering combined samplers to separate sampler+texture objects for SM 4.

Following patches (including prepending uniform copies resource components within struct parameters) in: https://gitlab.winehq.org/fcasas/vkd3d/-/commits/master6c

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
430 430 return type;
431 431 }
432 432
433 void hlsl_write_component_name(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffer,
434 const struct hlsl_ir_var *var, unsigned int index)
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 35deef6a
  • 2537 for (i = 0; i < component_count; ++i)
    2538 {
    2539 struct hlsl_type *component_type = hlsl_type_get_component_type(ctx, var->data_type, i);
    2540 struct hlsl_ir_var *new_var;
    2541 struct hlsl_ir_node *instr;
    2542 enum hlsl_regset regset;
    2543
    2544 if (!hlsl_type_is_resource(component_type))
    2545 continue;
    2546
    2547 regset = hlsl_type_get_regset(component_type);
    2548
    2549 if (offsets[regset] > var->regs[regset].bind_count)
    2550 continue;
    2551
    2552 if (var->objects_usage[regset][offsets[regset]].used)
  • Elizabeth Figura
  • Elizabeth Figura
  • Elizabeth Figura
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit e63985ea
  • 1984 /* Lower combined samples and sampler variables to synthesized separated textures and samplers.
    1985 * That is, translate SM1-style samples in the source to SM4-style samples in the bytecode. */
    1986 static bool lower_combined_samples(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
    1987 {
    1988 unsigned int i, k, path_len_to_sampler_array = 0;
    1989 struct hlsl_deref deref_to_sampler_array;
    1990 struct hlsl_ir_resource_load *sample;
    1991 struct hlsl_type *sampler_array_type;
    1992 struct vkd3d_string_buffer *name;
    1993 struct hlsl_ir_var *var;
    1994
    1995 if (instr->type != HLSL_IR_RESOURCE_LOAD)
    1996 return false;
    1997 sample = hlsl_ir_resource_load(instr);
    1998 if (sample->load_type != HLSL_RESOURCE_SAMPLE || sample->sampler.var)
    1999 return false;
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit e63985ea
  • 2001 /* Find the corresponding sampler array (or single sample component) in the path. */
    2002 sampler_array_type = sample->resource.var->data_type;
    2003 for (i = 0; i <= sample->resource.path_len; ++i)
    2004 {
    2005 if (hlsl_type_is_resource(sampler_array_type))
    2006 {
    2007 path_len_to_sampler_array = i;
    2008 break;
    2009 }
    2010 assert(i < sample->resource.path_len);
    2011 sampler_array_type = hlsl_get_element_type_from_path_index(ctx, sampler_array_type, sample->resource.path[i].node);
    2012 }
    2013 assert(hlsl_type_get_regset(sampler_array_type) == HLSL_REGSET_SAMPLERS);
    2014
    2015 hlsl_copy_deref(ctx, &deref_to_sampler_array, &sample->resource);
    2016 hlsl_strip_deref_suffix(ctx, &deref_to_sampler_array, path_len_to_sampler_array);
    • Comment on lines +2002 to +2016

      This seems very complicated, especially this deref manipulation.

      I'm rather tempted to propose that we just leave off handling anything but top-level sm1 variables for an hlsl_fixme(). For that matter, do we even have anything that actually needs this combined-sampler logic?

    • It's true it's a bit intricate (not that I know of any better way to do the same thing), but I don't think we should drop it. I know we already discussed that, but to me whether a feature is used somewhere or not shouldn't be a criterion to decide whether to have that feature or not (it can be a reasonable criterion to decide on what to concentrate development). In theory our shader compiler should be able to compile any shader that is accepted by the native compiler.

    • I understand that mindset, but I'm really not happy about maintaining complicated code that might never be used. It's the same reason we discourage implementing things in Wine that no application asks for.

      Perhaps more saliently, after doing a bit of testing, it seems that native logic here gets stupidly complicated, and this code isn't quite correct as-is. I don't think it's really worth trying to fix the code just for that.

    • If there are counterexamples, then we're in a different league, sure.

    • Francisco Casas changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • Author Developer

      I'm rather tempted to propose that we just leave off handling anything but top-level sm1 variables for an hlsl_fixme(). For that matter, do we even have anything that actually needs this combined-sampler logic?

      No, not that I remember. Okay, I simplified it so that it only supports samplers and sample arrays (it emits a fixme for samplers within structs), now it is closer to the original patch. There is a test that will remain as TODO.

      Perhaps more saliently, after doing a bit of testing, it seems that native logic here gets stupidly complicated, and this code isn't quite correct as-is. I don't think it's really worth trying to fix the code just for that.

      I agree that the native logic gets complicated (and the native compiler even emits its own fixmes in many cases), also, it is hard to know what it does support and what it doesn't. But I am curious about what is not correct of the code.

    • I don't remember exactly; unfortunately I didn't write down, but I was able to recreate this example

      struct apple
      {
        struct
        {
          sampler s[2];
          float4 pos;
        } s[2];
      } a;
      
      float4 main(float4 f : texcoord) : sv_target
      {
        return tex2D(a.s[1].s[1], a.s[1].pos);
      }

      which I believe yielded different RDEF information between native and (the old version of) this patch set.

      I feel like there might have been an example that also yielded different allocation, but I might be misremembering.

    • Author Developer

      I feel like there might have been an example that also yielded different allocation, but I might be misremembering.

      Note that after this series, I have a couple of patches that take care of implementing some sorting rules so that register allocation matches native. e.g. multi-register externs are allocated before (i.e. with lower register indexes) than single-register externs.

    • Please register or sign in to reply
    • From the subject of patch 5/6:

      track_object_components_usage() must be called twice:

      This kind of suggests that making it do two different things wasn't actually desirable after all.

    • Author Developer

      I added a patch to split the pass into two: track_object_components_sampler_dim() and track_object_components_usage().

    • Please register or sign in to reply
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
    • Resolved by Giovanni Mascellani

      Trying to run this shader:

      struct
      {
        Texture2D tex;
        sampler sam[2][2];
      } foo;
      
      float4 main() : sv_target
      {
          return 10 * foo.tex.Sample(foo.sam[0][0], float2(0, 0)) + tex2D(foo.sam[1][1], float2(0, 0));
      }

      triggers an assertion:

      vkd3d:fixme:spirv_compiler_get_descriptor_binding Could not find descriptor binding for type 0x3, space 0, registers [3:3], shader type 0.
      shader_runner:422: Section [test], line 139: Test failed: Failed to create state, hr 0x80004005.
      shader_runner: ../vkd3d/libs/vkd3d/state.c:1937: unsafe_impl_from_ID3D12PipelineState: Assertion `iface->lpVtbl == &d3d12_pipeline_state_vtbl' failed.
      Annullato

      This seems to be a missing feature in vkd3d, not a problem of vkd3d-shader, so it's not really related to this patch, but it seemed to be something worth mentioning.

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit e63985ea
  • 3225 3225 {
    3226 3226 var = extern_resources[i];
    3227 3227
    3228 string_offset = put_string(&buffer, var->name);
    3228 if (!strncmp(var->name, "<resource>", strlen("<resource>")))
    3229 string_offset = put_string(&buffer, var->name + strlen("<resource>"));
    3230 else
    3231 string_offset = put_string(&buffer, var->name);
    • Comment on lines +3228 to +3231

      I'm not sure I like this (specifically, emitting a variable name that it mangled or not depending on the name itself). I guess you're doing it to mimic native?

    • No, this is never visible in native. I originally wrote this part, and the motivation was to avoid creating multiple variables with the same name. In order to do it differently we'd need to adjust hlsl_add_var() and hlsl_get_var(), and we may also want to adjust dump_var().

    • Then I think I would be in favor to emit the <resource> as well.

    • Francisco Casas changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • Author Developer

      So, we can resolve this comment and !209 (comment 33393) as well, right?

    • Hmm, WRT !209 (comment 33393) I still see exactly the same line. Am I missing anything? Well, in the end I can live with it, it's technically not wrong, but I think it's brittle. It's easy to forget about which conventions are used and break things later. While a flag in a struct is much more explicit, and harder to inadvertently remove.

    • Then I think I would be in favor to emit the <resource> as well.

      I think I may have misinterpreted this comment, I assumed you were saying that you preferred the code as I wrote it? Or are you saying we should write out "" into the table instead of stripping it?

      If the latter—to clarify, by "not visible" I mean the "" part is never visible, that's an invention I added, basically working around code that I wasn't sure at the time could handle multiple variables with the same name (well, definitely can't, in some respects). Native does indeed have multiple variables with the same name in the RDEF table.

      I don't love the hack, though I still don't hate it either, and I can agree with the idea of just storing that as an extra part of the key.

      --

      Actually, what just occurred to me: along the same lines as some of my suggestions on this merge request, I'm not even sure we need to synthesize another variable. We could just reuse this variable (and set some flag telling RA that we need to allocate both sampler and texture variables for it?)

      That does mean we need special logic around sm4_register_from_deref() [which could potentially be a simple as passing a register set to that function?], which is a bit unfortunate but maybe ultimately not that unfortunate? What do you think?

    • Ok, I think I understand. I think I'd prefer if we kept two names for each variable, one for internal indexing and the other to be printed out in RDEF, rather than having that special exception if tpf.c, but I think I can tolerate it. I might become more vocal about it, though, if in the future the practice of prepending a variable name with a <tag> were to become more frequent.

      I'd still ask for adding some kind of is_separeted_resource flag to hlsl_ir_var rather than parsing the variable name in 9/9.

    • Author Developer

      Ok, I think I understand. I think I'd prefer if we kept two names for each variable, one for internal indexing and the other to be printed out in RDEF, rather than having that special exception if tpf.c, but I think I can tolerate it. I might become more vocal about it, though, if in the future the practice of prepending a variable name with a <tag> were to become more frequent.

      I like the idea of keeping two names for each variable. I would call them var->name and var->display_name.

      I'd still ask for adding some kind of is_separeted_resource flag to hlsl_ir_var rather than parsing the variable name in 9/9.

      I agree with this too.

    • I'd rather see just a name plus a flag, than an external and internal name.

      Honestly the more I think about my suggestion the more I like it; it seems like what the register set logic was built for. I was envisioning that sm1 bool/int/float would work that way as well, although I guess duplicating the variable there is an option too.

    • Author Developer

      Honestly the more I think about my suggestion the more I like it; it seems like what the register set logic was built for. I was envisioning that sm1 bool/int/float would work that way as well, although I guess duplicating the variable there is an option too.

      I have been giving it some thought of the idea of having the same variable for both the sampler part and the resource part of a lowered combined sample, and I don't think I like it very much.

      There are a couple of problems we may have:

      1. Seems that the allocation order of the texture part and the sampler part is not bonded together in the native compiler:

      Consider:

      Texture2D tex0;
      sampler sam0;
      
      sampler comb;
      
      Texture2D tex1;
      sampler sam1;
      
      float4 main() : sv_target
      {
          return tex2D(comb, float2(0, 0)) + tex0.Sample(sam0, float2(0, 0)) + tex1.Sample(sam1, float2(0, 0));
      }

      we get the following bindings:

      // sam0                              sampler      NA          NA             s0      1 
      // comb                              sampler      NA          NA             s1      1 
      // sam1                              sampler      NA          NA             s2      1 
      // comb                              texture  float4          2d             t0      1 
      // tex0                              texture  float4          2d             t1      1 
      // tex1                              texture  float4          2d             t2      1 

      the texture part of comb is allocated before tex0, but the sampler part of comb is allocated after sam0 (respecting declaration order), which shows that the texture parts of lowered combined samplers have to be moved first in the allocation order. I have a continuation patch that handle this in an easy way, just sorting the new created texture variable forward.

      If we keep the same variable however, we may need to sort variables between calls to allocate_objects() for different regsets, to ensure this behavior.

      1. The var->objects_usage[] array is created on hlsl_new_var() from the type->regsize[]. So we would either have to allocate memory for it once we discover that var is used as a combined sampler or we would have to preemptively set type->regsize[HLSL_REGSET_TEXTURES] the same as type->regsize[HLSL_REGSET_SAMPLERS] for all samplers in SM4.

      2. We are currently assuming that all resources belong to a single regset, so we would have to add additional checks in many places where hlsl_type_get_regset() is called, to know if we are interested in the texture or sampler part.

      1. Seems that the allocation order of the texture part and the sampler part is not bonded together in the native compiler:

      That doesn't seem like a blocker, though. We already do allocation in multiple passes; we could do a separate pass for combined samplers where necessary.

      1. The var->objects_usage[] array is created on hlsl_new_var() from the type->regsize[]. So we would either have to allocate memory for it once we discover that var is used as a combined sampler or we would have to preemptively set type->regsize[HLSL_REGSET_TEXTURES] the same as type->regsize[HLSL_REGSET_SAMPLERS] for all samplers in SM4.

      Or we mark internally that the variable is used as a combined sampler before RA (i.e. the same time when we're running lower_combined_samples, probably) and then set objects_usage based on that.

      1. We are currently assuming that all resources belong to a single regset, so we would have to add additional checks in many places where hlsl_type_get_regset() is called, to know if we are interested in the texture or sampler part.

      See, and this is where I have to once again state that hlsl_type_get_regset() has always seemed like a fundamentally broken function to me. It never had a clear boolean answer for structs, and I'm not convinced it can have a clear boolean answer for individual variables either.

      I'm not married to this idea of reusing variables, but I'm still not quite convinced it's the wrong idea either.

    • Author Developer

      Seems that the allocation order of the texture part and the sampler part is not bonded together in the native compiler:

      That doesn't seem like a blocker, though. We already do allocation in multiple passes; we could do a separate pass for combined samplers where necessary.

      Yep, that is a reasonable solution.

      The var->objects_usage[] array is created on hlsl_new_var() from the type->regsize[]. So we would either have to allocate memory for it once we discover that var is used as a combined sampler or we would have to preemptively set type->regsize[HLSL_REGSET_TEXTURES] the same as type->regsize[HLSL_REGSET_SAMPLERS] for all samplers in SM4.

      Or we mark internally that the variable is used as a combined sampler before RA (i.e. the same time when we're running lower_combined_samples, probably) and then set objects_usage based on that.

      A problem is that we first need to use var->objects_usage in track_object_components_sampler_dim() first, so that we know the sampler_dim of the new texture to be generated.

      But I have an alternate solution: we could add a "requires_separate_texture" field to the anonymous struct in objects_usage[][], storing the requirement for the texture resource allocation in objects_usage[HLSL_REGSET_SAMPLER][·].

      See, and this is where I have to once again state that hlsl_type_get_regset() has always seemed like a fundamentally broken function to me. It never had a clear boolean answer for structs, and I'm not convinced it can have a clear boolean answer for individual variables either.

      I agree in part now. IMO, we should get rid of hlsl_type_get_regset() except for when it is used on a particular deref (maybe, turning it into something like hlsl_deref_get_regset(), or extending deref->offset_regset to the whole lifetime of the deref).

      Currently we are using it for two things:

      1. As "the regset of the type of a value used by an instruction", which I think is the correct use. In theory, a deref shall never point to a struct after we have split copies.

      All these uses after the deref is lowered into a single offset can be replaced by deref->offset_regset. The uses before the lowering would still require to use the implementation of hlsl_type_get_regset() but on the type reached by the deref's path. We would have the guarantee that it is not a struct (unless we are doing something wrong).

      1. When we iterate over extern variables to either allocate registers or write the CTAB and RDEF sections. We sometimes use hlsl_type_is_resource() to ensure that they belong to a single regset.

      In all these cases we should assume that each variable doesn't necessarily belong to a single regset and not call the function. Instead, iterate over all regsets and check individually if the variable is allocated, or needs to be.


      The last change in particular is big so I would prefer to leave these things for part 4 of this series and upstream this MR as it is. If you think we should introduce them right away, that's fine, I hope I don't make these new patches too controversial though.

    • I'm not married to this idea of reusing variables, but I'm still not quite convinced it's the wrong idea either.

      I'm not entirely sure of what's your positive reason for reusing variables, though. I find Francisco's current solution reasonable. I had a few comments on some specific details, but the general architecture makes sense. I'd say that native behavior makes it rather natural to think a SM1-3 sampler as, secretly, a SM4 sampler plus a resource, so the transformation Francisco is introducing doesn't look hacky. OTOH using the same variable object for two different things doesn't feel very natural. I'm sure it can be made to work, but I see little value in reconsidering previous design choices for what doesn't really look like an enhancement in itself. Could you please explain what you like of your proposal?

    • I'm not entirely sure of what's your positive reason for reusing variables, though. I find Francisco's current solution reasonable. I had a few comments on some specific details, but the general architecture makes sense. I'd say that native behavior makes it rather natural to think a SM1-3 sampler as, secretly, a SM4 sampler plus a resource, so the transformation Francisco is introducing doesn't look hacky. OTOH using the same variable object for two different things doesn't feel very natural. I'm sure it can be made to work, but I see little value in reconsidering previous design choices for what doesn't really look like an enhancement in itself. Could you please explain what you like of your proposal?

      Without having tried it, it just seems simple and like it makes sense. The old code isn't complex either (well, until it starts needing to handle struct/array types) but it seems like it should be even simpler to do it this way. There's no need to add synthetic variables (and hence no internal flags to mark them as different from the other variables).

      It also lines up with my preconceived notion of how sm3 int/bool constants would work. Again, they don't actually have to work that way, but I feel like it makes a lot of sense.

    • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 78e71c4b
  • 3047 3047 {
    3048 3048 for (i = 0; i < type->reg_size[k]; ++i)
    3049 3049 {
    3050 /* Samplers are only allocated until the last used one. */
    3050 bool is_separated = !strncmp(var->name, "<resource>", strlen("<resource>"));
  • Francisco Casas added 9 commits

    added 9 commits

    • 295c24e4 - vkd3d-shader/hlsl: Allow derefs to provide the data_type.
    • 7a4f65e3 - vkd3d-shader/tpf: Introduce struct extern_resource.
    • 09a4edbe - vkd3d-shader/hlsl: Handle resource components individually for SM 5.0.
    • 56485a33 - tests: Add lowering combined samplers tests.
    • f4bfa3ce - vkd3d-shader/hlsl: Check is_uniform instead of HLSL_STORAGE_UNIFORM when validating object refs.
    • fadc0af9 - vkd3d-shader/hlsl: Introduce hlsl_new_synthetic_var_named().
    • f4dca94e - vkd3d-shader/hlsl: Separate tracking of sampler_dim and usage for object components.
    • 1bec4225 - vkd3d-shader/hlsl: Lower combined samplers to separate sampler and texture objects for SM4.
    • 95acdde6 - vkd3d-shader/hlsl: Don't allocate all texture registers for synthetic separated samplers.

    Compare with previous version

  • Francisco Casas mentioned in merge request !219 (merged)

    mentioned in merge request !219 (merged)

  • Giovanni Mascellani
  • Giovanni Mascellani
    • I couldn't try this since it does not apply any more, but one thing that I noticed is that 1D case should actually produce 2D resource declaration, and use (x, 0.5) as a coordinate.

    • Author Developer

      I see, albeit I am inclined to implement this as a new pass as in !219 (comment 34533)

      In this scheme, SM1 should keep the 1D resource loads, and just pass any value to src0.y when writing texld or texldp. One thing would remain, which is to allow SM1 samplers to be used both in tex1D() and tex2D() without calling them inconsistent.... Native SM4 just explodes:

      sampler sam;
      float4 main() : sv_target
      {
          return tex1D(sam, 0.0) + tex2D(sam, float2(0, 1));
      }
      internal error: compilation aborted unexpectedly
      
      compilation failed; no code produced
    • Please register or sign in to reply
  • Or maybe this could be done by functions directly. Except that forcing tex1D() to use 2D dim will also have to set y component for sm4+, which at the same should not be affected by projective division.

    Edited by Nikolay Sivov
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading