Skip to content
Snippets Groups Projects

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

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

Indexing with non-constants offsets requires relative addressing in SM4. In assembly, this is written like in the following example:

x1[r1.x + 3]

The first part of this patch series only includes support for indexing vectors with non-constant indexes.

Following patches in https://gitlab.winehq.org/fcasas/vkd3d/-/commits/nonconst-offsets-3.


Non-constant indexing of vectors cannot be implemented with relative addressing in SM4 because this indexation cannot be performed at the level of register-components, only whole registers.

Mathematical operations must be used instead.

For floats, the native compiler seems to index an identity matrix, to get the i-th column, and then proceedes to compute the dot product between that column and the vector. For ints, bit operations seem to be performed.

While probably less efficient, this implementation complies with the type-checking at the IR level and when writing bytecode.

Merge request reports

Checking pipeline status.

Approved by

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Jun 7, 2023 8:46pm UTC)

Merge details

  • Changes merged into master with ebf75735.
  • Deleted 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
15 draw quad
16 probe all rgba (12.0, 12.0, 12.0, 12.0)
17 uniform 0 float 2
18 draw quad
19 probe all rgba (13.0, 13.0, 13.0, 13.0)
20 uniform 0 float 3
21 draw quad
22 probe all rgba (14.0, 14.0, 14.0, 14.0)
23
24
25 [pixel shader todo]
26 uniform float2 i;
27
28 float4 main() : sv_target
29 {
30 float4 f[3] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12};
  • Does it matter if this is a const array or a uniform? I suppose either way we can obsolete !189 (merged) with this MR, whether we need a separate test or not.

  • Author Developer

    Currently it doesn't matter only because our strategy is copying everything into temps at the beginning of the shader and (if they are output semantics or uniforms) copying from the temps back to the variables at the end of the shader.

    However, I think it is good to retain both tests. Merging depending on which MR goes first.

  • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit c9c10f02
  • 2051 for (i = 0; i < type->dimx; ++i)
    2052 {
    2053 unsigned int s = hlsl_swizzle_from_writemask(1 << i);
    2054
    2055 if (!(comps[i] = hlsl_new_swizzle(ctx, s, 1, mult, &instr->loc)))
    2056 return false;
    2057 list_add_before(&instr->entry, &comps[i]->entry);
    2058 }
    2059
    2060 res = comps[0];
    2061 for (i = 1; i < type->dimx; ++i)
    2062 {
    2063 if (!(res = hlsl_new_binary_expr(ctx, HLSL_OP2_ADD, res, comps[i])))
    2064 return false;
    2065 list_add_before(&instr->entry, &res->entry);
    2066 }
    • Comment on lines +2047 to +2066

      I suppose that you're spelling out the dot product so that it works with any numeric type? I'd rather emit an actual DOT here and then implement DOT lowering for integral types: you make the code cleaner and implement another feature for free.

      Also, notice that the current code triggers an assertion when indexing bool4. I think that's due to the introduction of a bool cast once they're assumed to have already been lowered. I guess this pass should happen a bit earlier (though, to be fair, it's always difficult to track the pass dependencies and assumptions).

    • Author Developer

      I suppose that you're spelling out the dot product so that it works with any numeric type? I'd rather emit an actual DOT here and then implement DOT lowering for integral types: you make the code cleaner and implement another feature for free.

      That is a good idea! I did just that. Note that HLSL_OP2_DOT is not defined for bools, so bool vectors are a special case I had to handle with HLSL_OP2_LOGIC_AND and HLSL_OP2_LOGIC_OR.

      Also, notice that the current code triggers an assertion when indexing bool4. I think that's due to the introduction of a bool cast once they're assumed to have already been lowered. I guess this pass should happen a bit earlier (though, to be fair, it's always difficult to track the pass dependencies and assumptions).

      I moved lower_nonconstant_vector_derefs() up. Also, I realized that this shouldn't be a transform_derefs(·) pass, but just a hlsl_transform_ir(·) pass on hlsl_ir_loads. transform_derefs(·) will still be used in following patches though, so I will keep its introduction patch here.

    • That is a good idea! I did just that. Note that HLSL_OP2_DOT is not defined for bools, so bool vectors are a special case I had to handle with HLSL_OP2_LOGIC_AND and HLSL_OP2_LOGIC_OR.

      Maybe my mathematician side is prevailing too much for Zeb's tastes, but I would just decide that DOT is defined for bool too, simply using AND instead of MUL and OR instead of ADD (unless I am missing something, you can basically use the same lower_int_dot() implementation, just using the appropriate HLSL_OP2 constants). Notice that this is different of how the dot() intrinsic work, but that's not necessarily a problem because the bool dot product is immediately casted to int while parsing (inside expr_common_base_type()).

      Edited by Giovanni Mascellani
    • Author Developer

      That would save some code, but it seemed odd to me to make the space between

      hlsl_transform_ir(ctx, lower_nonconstant_vector_derefs, body, NULL);

      and

      hlsl_transform_ir(ctx, lower_int_dot, body, NULL);

      the only place in the whole codebase where HLSL_OP2_DOT for bools can exist.

      I will change it if nobody disagrees though.

    • We don't have tests for this, but dot() of bools is uniquely supposed to return float.

      I don't necessarily object to having HLSL_OP2_DOT mean something other than dot(), although I don't know that it's great. The idea that dot() of bools returns a bool is weird, too. I don't hate either one but I'm not sure it's better than the existing code either.

    • Author Developer

      We don't have tests for this, but dot() of bools is uniquely supposed to return float.

      Hmm, I think we are not doing that then. As Giovanni mentioned, we are making them return int.

      I don't necessarily object to having HLSL_OP2_DOT mean something other than dot(), although I don't know that it's great. The idea that dot() of bools returns a bool is weird, too. I don't hate either one but I'm not sure it's better than the existing code either.

      Since it allows to remove a chunk of code, I am generating the DOT for bools now. It is lowered by the following lower_int_dot() pass. I also added a comment:

      /* Note: We may be creating a DOT for bool vectors here, which we need to lower to
       * LOGIC_OR + LOGIC_AND. */
    • Francisco Casas changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • the only place in the whole codebase where HLSL_OP2_DOT for bools can exist.

      Though notice that it's not by design. If we find something else that sensibly generates a DOT of bools, then we can use it there too.

      At any rate, I fully understand that my way of reasoning might sound a bit peculiar here, and definitely the SLOC difference isn't that large, so no hard feelings if you and Zeb decide in the end that I can keep my algebraic mutterings somewhere else.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • Francisco Casas added 5 commits

    added 5 commits

    • ed7a8b30 - tests: Test non-constant vector indexing.
    • ce803620 - tests: Test indexing temps.
    • 9b2f7398 - vkd3d-shader/hlsl: Introduce transform_derefs().
    • 134ac995 - vkd3d-shader/hlsl: Lower dot for integral types.
    • aa7a8ccb - vkd3d-shader/hlsl: Support non-constant vector indexing.

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 134ac995
  • 2390 2390 }
    2391 2391
    2392 static bool lower_int_dot(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
    2393 {
    2394 struct hlsl_ir_node *arg1, *arg2, *mult, *comps[4] = {0}, *res;
    2395 struct hlsl_type *type = instr->data_type;
    2396 struct hlsl_ir_expr *expr;
    2397 unsigned int i, dimx;
    2398
    2399 if (instr->type != HLSL_IR_EXPR)
    2400 return false;
    2401 expr = hlsl_ir_expr(instr);
    2402
    2403 if (expr->op != HLSL_OP2_DOT)
    2404 return false;
    2405 if (type->base_type == HLSL_TYPE_FLOAT)
    • Would it be better to avoid that deref path manipulation by doing this lowering in lower_index_loads()?

      If not, I'd rather have a dedicated interface for "get the immediate parent of this deref". I really want to make as little code touch the "path" fields as possible.

    • Author Developer

      Would it be better to avoid that deref path manipulation by doing this lowering in lower_index_loads()?

      I remember to have considered this, but:

      • Intertwining the logic for indexing column-major matrices with the logic for non-constant vector dereferences may get messy.
      • I just remembered that we probably want to run this pass after folding constant expressions, to avoid adding instructions that we can't fold, e.g. just because of an unfolded cast in the index. This, unless we make the passes smart enough so they can fold x + 0 = x and x * 0 = 0.

      So, I moved the pass after the constant folding. I am afraid that that also requires to go through the lower_casts_to_bool() and lower_int_dot() passes a second time.

      If not, I'd rather have a dedicated interface for "get the immediate parent of this deref". I really want to make as little code touch the "path" fields as possible.

      I changed hlsl_new_load_partial_path() to hlsl_new_load_path_parent(). I wonder if it will be possible to keep a low level of path manipulation for the resources support though.

    • I just remembered that we probably want to run this pass after folding constant expressions, to avoid adding instructions that we can't fold, e.g. just because of an unfolded cast in the index. This, unless we make the passes smart enough so they can fold x + 0 = x and x * 0 = 0.

      I came to thinking the same while reviewing. Even more, I might be not computationally optimal, but in the end the compiler structure might be simpler if we just lower all vector indexing operations to a dot product and them let constant folding and other passes recover the indexing operations that can be made constant.

      Just saying, though, I'd first have this MR accepted and then this can be revisited later.

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

    added 2 commits

    • 782557ea - vkd3d-shader/hlsl: Lower dot for non-float types.
    • 95598019 - vkd3d-shader/hlsl: Support non-constant vector indexing.

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit 95598019
  • 2038 list_add_before(&instr->entry, &c->node.entry);
    2039
    2040 operands[0] = swizzle;
    2041 operands[1] = &c->node;
    2042 if (!(eq = hlsl_new_expr(ctx, HLSL_OP2_EQUAL, operands,
    2043 hlsl_get_vector_type(ctx, HLSL_TYPE_BOOL, type->dimx), &instr->loc)))
    2044 return false;
    2045 list_add_before(&instr->entry, &eq->entry);
    2046
    2047 if (!(eq = hlsl_new_cast(ctx, eq, type, &instr->loc)))
    2048 return false;
    2049 list_add_before(&instr->entry, &eq->entry);
    2050
    2051 op = HLSL_OP2_DOT;
    2052 if (type->dimx == 1)
    2053 op = type->base_type == HLSL_TYPE_BOOL ? HLSL_OP2_LOGIC_AND : HLSL_OP2_MUL;
    • Comment on lines +2052 to +2053

      TBH I wouldn't even dislike deciding that DOT is defined for scalars, with the obvious semantics. Which might even be my fault if it's not. But no need to further overload this MR with that.

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

    approved this merge request

  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 95598019
  • 2410 2486 arg2 = expr->operands[1].node;
    2411 2487 assert(arg1->data_type->dimx == arg2->data_type->dimx);
    2412 2488 dimx = arg1->data_type->dimx;
    2489 is_bool = type->base_type == HLSL_TYPE_BOOL;
    2413 2490
    2414 if (!(mult = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, arg1, arg2)))
    2491 if (!(mult = hlsl_new_binary_expr(ctx, is_bool ? HLSL_OP2_LOGIC_AND : HLSL_OP2_MUL, arg1, arg2)))
  • Francisco Casas added 2 commits

    added 2 commits

    • 960ef4fe - vkd3d-shader/hlsl: Lower dot for non-float types.
    • 74888ad5 - vkd3d-shader/hlsl: Support non-constant vector indexing.

    Compare with previous version

  • Francisco Casas added 60 commits

    added 60 commits

    • 74888ad5...7c360330 - 55 commits from branch wine:master
    • 5b22cd9d - tests: Test non-constant vector indexing.
    • d2db1d30 - tests: Test indexing temps.
    • 48c2ae4e - vkd3d-shader/hlsl: Introduce transform_derefs().
    • b8b538b3 - vkd3d-shader/hlsl: Lower dot for non-float types.
    • 09b3a0ec - vkd3d-shader/hlsl: Support non-constant vector indexing.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet mentioned in merge request !223 (merged)

    mentioned in merge request !223 (merged)

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading