Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Fold basic idempotencies.

Merged Francisco Casas requested to merge fcasas/vkd3d:idempotencies into master
2 unresolved threads

While not strictly necessary, this pass allows to remove some unnecessary instructions.

For instance:

float4 a;

float4 main() : sv_target
{
    float4 zero = {0, 0, 0, 0};
    float4 one = {1, 1, 1, 1};

    return a + zero + a * one + zero * one;
}

Compiles to:

ps_4_0
dcl_constantbuffer cb0[1], immediateIndexed
dcl_output o0.xyzw
dcl_temps 1
mov r0.xyzw, cb0[0].xyzw
add r0.xyzw, r0.xyzw, r0.xyzw
mov o0.xyzw, r0.xyzw
ret

Instead of

ps_4_0
dcl_constantbuffer cb0[1], immediateIndexed
dcl_output o0.xyzw
dcl_temps 2
mov r0.xyzw, cb0[0].xyzw
add r1.xyzw, r0.xyzw, l(0.000000, 0.000000, 0.000000, 0.000000)
mul r0.xyzw, r0.xyzw, l(1.000000, 1.000000, 1.000000, 1.000000)
add r0.xyzw, r1.xyzw, r0.xyzw
add r0.xyzw, r0.xyzw, l(0.000000, 0.000000, 0.000000, 0.000000)
mov o0.xyzw, r0.xyzw
ret

Merge request reports

Merge request pipeline #25866 skipped

Merge request pipeline skipped for ee735e8e

Merged by Alexandre JulliardAlexandre Julliard 11 months ago (Apr 30, 2024 9:13pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
      • /* The expression must have only one constant argument and only one non-constant argument. */

      Conceptually, they could both be constants, though. Sure, those will already be taken care of, but we don't need to go to lengths to validate that.

    • In my opinion those cases should always be handled by hlsl_fold_constant_exprs() so that the logic here can be simplified.

      If we allow to handle cases with two constants, we would have to test both for being zero (or one) on each supported op.

    • They will, but isn't it simpler to not care whether one or the other is a constant?

      If the first argument is a constant, use it, otherwise, if the second argument is a constant, use it, otherwise, return false.

    • If we find constant folding problems it might be beneficial to immediately discard this folding of identities as a suspect. Also the asymmetry of how the cases with two constants will be handled bothers me a little, we would be folding 0 + 8 but no 8 + 0. And there isn't much gain in simplification since we also have to check that the expression indeed has two operands in the new version.

      Anyways, these are little things and the code seems more readable, so I applied this suggestion.

    • Please register or sign in to reply
    • This also feels like a lot of boilerplate for a pass that's only going to be useful for a couple ops?

    • I don't think that's particularly problematic. Also, other operations could be added at some point (multiplication by zero, XOR and OR with zero, AND with one, AND with zero, subtraction to and from zero, division by and from one when RCP is available, maybe others).

    • Eh, maybe I am underestimating the number of passes it's useful for...

    • Thinking more about it, this machinery could be useful to allow DCE to fold short-circuit evaluation, for false and x and true or x.

      In the native compiler it seems quite aggressive, not even resource loads are safe:

      Texture2D tex;
      
      float4 main() : sv_target
      {
          if (false && tex.Load(int3(0, 0, 0)).x == 2.0)
              return 0;
          return 1;
      }
    • Hmm, I don't remember whether HLSL has the same short circuit rules as C, but if it does the above code should already compile (at the frontend level) to something like:

      if (false)
      {
          if (tex.Load(...))
              return 0;
      }
      return 1;

      And here you don't need to fold any identity, this is just dropping statically unreachable branches (which I guess we already have, don't we?). Also notice that here the Load() result is unused and I don't think Load() can have any side effect, so the compiler might just be dropping it independently of short circuit rules.

    • HLSL short circuits in sm6, but not sm1/sm4. But I think that's something we need to take care of at an earlier point.

    • You are right, I should have tried with an UAV store.

      RWTexture2D<float> uav;
      
      float4 main() : sv_target
      {
          if (false && (uav[int2(0, 0)] = 5))
              return 0;
          return 1;
      }

      In this case the store doesn't get deleted.

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

    approved this merge request

  • Giovanni Mascellani unapproved this merge request

    unapproved this merge request

  • Oh, I was forgetting: could you also add your test to the test suite?

    Yeah. Note that I think these particular ones are generally safe, but some of the other ones mentioned like multiplication by zero may not be under D3DCOMPILE_IEEE_STRICTNESS.

  • added 3 commits

    • 8b8569cb - tests: Test (+0) and (*1) indentities.
    • bcb9a211 - vkd3d-shader/hlsl: Fold x + 0 identities.
    • 0e7889b7 - vkd3d-shader/hlsl: Fold x * 1 identities.

    Compare with previous version

  • added 3 commits

    • d104213f - tests: Test x + 0 and x * 1 indentities.
    • adb9e37a - vkd3d-shader/hlsl: Fold x + 0 identities.
    • ffd0deb7 - vkd3d-shader/hlsl: Fold x * 1 identities.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • added 2 commits

    • 975caf31 - vkd3d-shader/hlsl: Fold x + 0 identities.
    • b9085323 - vkd3d-shader/hlsl: Fold x * 1 identities.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 101 commits

    • b9085323...13e14919 - 98 commits from branch wine:master
    • c43c900a - tests: Test x + 0 and x * 1 indentities.
    • b14f935d - vkd3d-shader/hlsl: Fold x + 0 identities.
    • ee735e8e - vkd3d-shader/hlsl: Fold x * 1 identities.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading