Skip to content
Snippets Groups Projects

hlsl: ps_1_* outputs.

Merged Elizabeth Figura requested to merge zfigura/vkd3d:pr0 into master
2 unresolved threads

Merge request reports

Merge request pipeline #10138 skipped

Merge request pipeline skipped for 8b57a612

Approved by

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (May 3, 2023 8:38pm UTC)

Merge details

  • Changes merged into master with 8b57a612.
  • Did not delete the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
2581 size_t i;
2576 2582
2577 for (i = 0; i < 4; ++i)
2583 for (i = 0; i < allocator->count; ++i)
2578 2584 {
2579 if (allocator->regs[component_idx + i].last_read <= first_write)
2580 {
2581 writemask |= 1u << i;
2582 if (++count == reg_size)
2583 return writemask;
2584 }
2585 const struct allocation *allocation = &allocator->allocations[i];
2586
2587 /* We do not overlap if first write == last read:
2588 * this is the case where we are allocating the result of that
2589 * expression, e.g. "add r0, r0, r1". */
  • Comment on lines +2587 to +2589

    I'm confused by this comment: doesn't that example show that you can in fact use the same register as both source and destination?

  • Author Developer

    Right, exactly, the point is that doesn't count as an overlap and hence that register isn't in use. Maybe I need to rephrase the comment to be worded less confusingly...

  • Oh I see, not sure why it confused me as it is. I guess I'd have expected the following if condition to have a "first_write <= allocation->last_read" to match?

  • In any case, I don't think you need to do anything about it.

  • I felt sort of the same confusion. Part of it is that "We do not overlap" can mean two things: "There is not overlap when fw == lr, so we can proceed" (the intended meaning) and "We (the programmers) do not want to create an overlap by accepting this case in which fw == lr, so we cannot proceed". I guess this might be a general problem with ergative verbs with an unclear pronoun (and I generally interpret "we" as the programmers in program comments).

    "There is no overlap" feels much clearer to me.

    The second instance of "we" ("we are allocating") is probably more clear, but maybe "we are" can be dropped there too without sacrificing grammaticality and without requiring the random reader to figure out who is "we".

  • Please register or sign in to reply
  • I haven't fully and thoroughly reviewed this MR but it does seem to be generally fine. Note to the fellow reviewer: the new get_available_writemask() is a pretty different function from the old one (WRT how it's used mostly), it just happens to cover the same "idea" and thus share the same name.

  • Elizabeth Figura added 31 commits

    added 31 commits

    • bf3c1862...b46df551 - 27 commits from branch wine:master
    • 3c5e9f08 - vkd3d-shader/hlsl: Rename struct liveness to struct register_allocator.
    • 83e1b8f6 - vkd3d-shader/hlsl: Avoid leaking the allocator register map in allocate_const_registers().
    • aa8111b4 - vkd3d-shader/hlsl: Rewrite the register allocator to allow allocating in multiple passes.
    • 24cbee07 - vkd3d-shader/hlsl: Map the colour output for ps_1_* to r0.

    Compare with previous version

    • The MR is ok for me, except for the little remark about comment wording.

      I just wanted to notice that the new algorithm has higher computational complexity than before, because get_available_writemask() used to be constant time and it's now linear in the number of register allocations. This already causes a measurable performance hit in a synthetic but still relatively simple shader as this:

      uniform float4x4 x;
      uniform float4x4 y;
      
      float4 main(float4 pos : sv_position) : sv_target
      {
          float4x4 a = mul(mul(y, x), mul(x, x));
          float4x4 b = mul(mul(y, x), mul(y, x));
          float4x4 c = mul(mul(y, y), mul(x, x));
          float4x4 d = mul(mul(y, y), mul(y, x));
      
          float4 ret = 0.0;
          ret += a[0] - b[0] * c[0] / d[0];
          ret += a[1] - b[1] * c[1] / d[1];
          ret += a[2] - b[2] * c[2] / d[2];
          ret += a[3] - b[3] * c[3] / d[3];
      
          return ret;
      }

      Here I am leveraging mul() to create a lot of temporaries and summing everything to prevent DCE from optimizing too much. On my computer a shader runner that just compiles this (doesn't execute it) takes 0.1 seconds before this MR and 0.11 seconds after it.

      I don't claim any significance for my random microbenchmark experiment, so I don't think it's necessary to change the MR, but when and if we'll be harvesting for performances in the HLSL compiler let's remember to have a look here.

    • Author Developer

      We could probably do better by just recording allocations for the few cases where we need to reserve, and then using the old pass for everything else. But it's probably not worth rewriting this again until we see evidence it matters.

    • Or simply use a more efficient data structure for recording allocations. I agree there is no need to do that now.

    • Please register or sign in to reply
  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 59 commits

    added 59 commits

    • 24cbee07...7917a682 - 55 commits from branch wine:master
    • c57ac0b2 - vkd3d-shader/hlsl: Rename struct liveness to struct register_allocator.
    • 71d8ff85 - vkd3d-shader/hlsl: Avoid leaking the allocator register map in allocate_const_registers().
    • b2959739 - vkd3d-shader/hlsl: Rewrite the register allocator to allow allocating in multiple passes.
    • 8b57a612 - vkd3d-shader/hlsl: Map the colour output for ps_1_* to r0.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading