Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Support non-constant offset dereferences, v2, part 1.

Merged Francisco Casas requested to merge fcasas/vkd3d:nonconst-offsets-8-part into master

First part of the continuation of the implementation of non-constant offset dereferences (a.k.a. relative addressing) for SM4, now that we use vsir registers in tpf.c.

As a quick recap: while parsing HLSL we are expressing derefs as paths, and then we are lowering these paths into a single offset node (which is closer to the bytecode) using the replace_deref_path_with_offset() pass, right before register allocation.

This first part of the series splits this offset node into 2 parts:

  • A constant uint, which will be called hlsl_deref.offset_const.
  • A non-hlsl_ir_constant offset node that will only be present when we need relative addressing, that we will end up calling hlsl_deref.offset_rel.

Both these fields will be analog to the ones used in vsir register indexes, vkd3d_shader_register_index.rel_addr and vkd3d_shader_register_index.offset respectively, which is something we need for the second part of this series.

The following patches are in my nonconst-offsets-8 branch, if something is not clear in this series, it may be worth skimming through them.

Supersedes !229 (closed).

Edited by Francisco Casas

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
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Small typo in "vkd3d-shader/hlsl: Absorv hlsl_ir_constant deref offsets into offset_const.": "Absorv" instead of "Absorb".

  • Giovanni Mascellani
  • Mostly ok, except a handful of random comments and a slighter large issue in the last commit.

  • Francisco Casas changed the description

    changed the description

  • Francisco Casas added 8 commits

    added 8 commits

    • 3e1a4241 - vkd3d-shader/hlsl: Clean-up instruction block for offset node creation.
    • af4e914e - vkd3d-shader/hlsl: Introduce hlsl_deref_is_lowered() helper.
    • 9a99b53c - vkd3d-shader/hlsl: Split deref-offset into a node and a constant uint.
    • 25068344 - vkd3d-shader/hlsl: Express deref->offset in whole registers.
    • 15190c2f - vkd3d-shader/hlsl: Absorb hlsl_ir_constant deref offsets into const_offset.
    • 8ca56719 - vkd3d-shader/hlsl: Rename hlsl_deref.offset to hlsl_deref.rel_offset.
    • d44df9ce - vkd3d-shader/hlsl: Mark vars that require non-constant dereferences.
    • d037d4fd - vkd3d-shader/tpf: Declare indexable temps.

    Compare with previous version

  • Author Developer

    Oh, I forgot to print a TRACE() message when the indexable vars are allocated. Adding it now...

  • Francisco Casas added 1 commit

    added 1 commit

    • cb8778b6 - vkd3d-shader/tpf: Declare indexable temps.

    Compare with previous version

  • Giovanni Mascellani resolved all threads

    resolved all threads

  • Giovanni Mascellani approved this merge request

    approved this merge request

    • I've been sitting on this because I'm still not sure about 4/9 and 5/9, when we're going to be feeding this through a lower level IR that's better equipped to handle those kinds of transformations. I still feel like this is going in the wrong direction. I don't want to block implementing an important feature forever, but I also don't want that refactoring to never happen. I suppose I can accept this as long as we are committed to fixing this the right way eventually, and not putting it off forever just because it's hard.

      I'm also bothered by 3/9. hlsl_deref_is_lowered() is used in two places:

      • hlsl_deref_get_type(), which feels like it should just have "if (deref->data_type) return deref->data_type;"

      • dump_deref(), which I'm not sure why it needs to call that function at all? Why isn't that just "else"?

    • Author Developer

      Alright, I will work on lowering HLSL to vsir in the following months, probably trying to conquer that bridge from both sides.

      From what we talked about, on one part we want tpf.c and d3dbc.c to not receive any HLSL IR, and on the other we want hlsl_codegen.c to output vsir. And we will probably will want two passes that translate from general vsir to the specific subset of vsir compatible with these formats, and the validation passes too.

      Regarding hlsl_deref_is_lowered(), I would like to keep it since it doesn't hurt (doesn't require ctx), its name makes is clear what we are checking for, instead of just checking for deref->data_type (which wouldn't be enough if we had accepted !343 (closed)).

      Additionally, in further passes I introduce many asserts like

      assert(!hlsl_deref_is_lowered(lhs));
      • hlsl_deref_get_type(), which feels like it should just have "if (deref->data_type) return deref->data_type;"

      We can do that, but the current version has the benefit of explaining that deref->data_type is not used if the deref is not lowered yet. That deref->data_type is the way that we check that is just a coincidence.

      *dump_deref(), which I'm not sure why it needs to call that function at all? Why isn't that just "else"?

      Because there is the possibility of a path deref to have path_len=0, if it is a direct reference to the variable, and I think we don't want to write those as [0] or [[]].

      Well, this is a pretty small thing, I can totally replace the hlsl_deref_is_lowered() calls with deref->data_type checks if you want.

    • Regarding hlsl_deref_is_lowered(), I would like to keep it since it doesn't hurt (doesn't require ctx), its name makes is clear what we are checking for, instead of just checking for deref->data_type (which wouldn't be enough if we had accepted !343 (closed)).

      Additionally, in further passes I introduce many asserts like

      assert(!hlsl_deref_is_lowered(lhs));
      • hlsl_deref_get_type(), which feels like it should just have "if (deref->data_type) return deref->data_type;"

      We can do that, but the current version has the benefit of explaining that deref->data_type is not used if the deref is not lowered yet. That deref->data_type is the way that we check that is just a coincidence.

      Yeah, that makes enough sense to me.

      *dump_deref(), which I'm not sure why it needs to call that function at all? Why isn't that just "else"?

      Because there is the possibility of a path deref to have path_len=0, if it is a direct reference to the variable, and I think we don't want to write those as [0] or [[]].

      Well, this is a pretty small thing, I can totally replace the hlsl_deref_is_lowered() calls with deref->data_type checks if you want.

      This one still bothers me. Neither [0] nor [[]] is actually wrong in that case, but moreover, I'd expect that if we're using hlsl_deref_is_lowered() then it should look something like

      if (hlsl_deref_is_lowered()) { // dump the offset } else { // dump the path }

      rather than this else-if logic we have.

    • Author Developer

      This one still bothers me. Neither [0] nor [[]] is actually wrong in that case, but moreover, I'd expect that if we're using hlsl_deref_is_lowered() then it should look something like

      if (hlsl_deref_is_lowered()) { // dump the offset } else { // dump the path }

      rather than this else-if logic we have.

      For the record, currently we are not writing [[]] for unlowered paths that have length 0, which I think is good to not overload the HLSL dump, so I think there is no need to change that behavior.

      I agree that separating the lowered/unlowered cases first makes it more readable, albeit I put the unlowered case first because it is what happens first. If that makes sense.

    • Please register or sign in to reply
  • Francisco Casas added 6 commits

    added 6 commits

    • 9aa7455a - vkd3d-shader/hlsl: Split deref-offset into a node and a constant uint.
    • c970a51d - vkd3d-shader/hlsl: Express deref->offset in whole registers.
    • be684083 - vkd3d-shader/hlsl: Absorb hlsl_ir_constant deref offsets into const_offset.
    • 2a4f9572 - vkd3d-shader/hlsl: Rename hlsl_deref.offset to hlsl_deref.rel_offset.
    • cf179d76 - vkd3d-shader/hlsl: Mark vars that require non-constant dereferences.
    • 8bd475a9 - vkd3d-shader/tpf: Declare indexable temps.

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading