Skip to content
Snippets Groups Projects

hlsl: Function inlining.

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

This is the last piece that's needed for function calls to basically work.

Merge request reports

Checking pipeline status.

Merged by Alexandre JulliardAlexandre Julliard 2 years ago (Feb 13, 2023 9:20pm 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
551 * the CF instruction, shove it into an if block, and then lower that if
552 * block.
553 *
554 * (We could return a "did we make progress" like transform_ir() and run
555 * this pass multiple times, but we already know the only block that still
556 * needs addressing, so there's not much point.)
557 *
558 * If we're inside of a loop CF block, we again do things differently. We
559 * already turned any returns into breaks. If the block we just processed
560 * was conditional, then "break" did our work for us. If it was a loop,
561 * we need to propagate that break to the outer loop.
562 */
563
564 LIST_FOR_EACH_ENTRY_SAFE(instr, next, &block->instrs, struct hlsl_ir_node, entry)
565 {
566 if (instr->type == HLSL_IR_CALL)
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 6a1c973f
  • 564 LIST_FOR_EACH_ENTRY_SAFE(instr, next, &block->instrs, struct hlsl_ir_node, entry)
    565 {
    566 if (instr->type == HLSL_IR_CALL)
    567 {
    568 struct hlsl_ir_call *call = hlsl_ir_call(instr);
    569
    570 lower_return(ctx, call->decl, &call->decl->body, false);
    571 }
    572 else if (instr->type == HLSL_IR_IF)
    573 {
    574 struct hlsl_ir_if *iff = hlsl_ir_if(instr);
    575
    576 lower_return(ctx, func, &iff->then_instrs, in_loop);
    577 lower_return(ctx, func, &iff->else_instrs, in_loop);
    578
    579 if (func->early_return_var)
    • I think I would find it more legible if we stipulated that early_return_var always exists, so you can skip a few branches in this function, which is already relatively complicated. This would leave in some more dead code, but hopefully there will eventually be a dead if elimination pass that gets rid of it.

      That said, nice function! Before reading that I thought it would have been much more of a pain.

    • From what I understood, early_return_var is only created for the current function once a return is found inside it, so it doesn't exist always.

    • Yeah, my proposal is to have it exist always. It could be created with the function itself, I guess. This way you can simplify a little bit a function which is already quite complex.

    • Hmm. After 5/6 it's only actually checked in one place. I suppose we could still create it in hlsl_func_decl() though? Not sure whether it makes a big difference.

    • I'm still in favor of creating it in hlsl_new_func_decl(). Agreed, it doesn't make a big difference, but it's one less thing to track when reading lower_return(). Though I can live with it as it is right now.

    • I don't have a preference personally, so I went ahead and changed it in v3.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit eb5d7acf
  • 12 12 RWTexture2D<float4> u;
    13 13
    14 void func(inout float4 f)
    14 void func(out float4 f)
    15 15 {
    16 f.xz += 0.1;
    16 f.x = 0.1;
    17 f.y = 0.2;
    18 f.z = 0.3;
    19 f.w = 0.4;
    17 20 }
    18 21
    19 22 [numthreads(1, 1, 1)]
    20 23 void main()
    21 24 {
    22 25 func(u[uint2(0, 0)].yzwx);
    • Seems that this is another case of discrepancy between the behavior of fxc 9.29 and fxc 10.1. The test being deleted in this patch worked for me using the latter (profile cs_5_0) so, at least in principle, I don't think it should be deleted.

    • The problem isn't that it doesn't compile, it's that it requires an optional d3d11/d3d12 feature to run. (It probably works on 47 and not on earlier versions because earlier versions predate that d3d11 version.) In practice it also fails in the Vulkan backend for basically that reason. We could add support in the shader runner [require] block to check for that feature, but it seemed easier to just change the test; it should still check the same things I was trying to test in the first place (i.e. swizzles work, in/out parameters work...)

    • Please register or sign in to reply
  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit 7a6da817
  • 2685 2690 return VKD3D_ERROR_NOT_IMPLEMENTED;
    2686 2691 }
    2687 2692
    2688 if (!(entry_func = hlsl_get_func_decl(&ctx, entry_point)))
    2693 if ((func = hlsl_get_function(&ctx, entry_point)))
    2694 {
    2695 RB_FOR_EACH_ENTRY(decl, &func->overloads, struct hlsl_ir_function_decl, entry)
    2696 {
    2697 if (!decl->has_body)
    2698 continue;
    2699 entry_func = decl;
    2700 break;
    2701 }
    • Comment on lines +2695 to +2701

      Since, in case of multiple overloads with definitions, we are still selecting the one that comes first in the order given by compare_function_decl_rb() -- which depends on the parameter types -- but in !65 (merged) it was mentioned that in native it depends on the .dll version and the order in which the declarations appear, I would suggest displaying a FIXME:

              RB_FOR_EACH_ENTRY(decl, &func->overloads, struct hlsl_ir_function_decl, entry)
              {
                  if (!decl->has_body)
                      continue;
                  if (entry_func)
                  {
                      FIXME("Multiple valid entry point definitions.\n");
                      break;
                  }
                  entry_func = decl;
              }
    • Please register or sign in to reply
  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit 6a1c973f
  • 429 429 * Not to be confused with the function parameters! */
    430 430 unsigned int attr_count;
    431 431 const struct hlsl_attribute *const *attrs;
    432 struct hlsl_ir_var *early_return_var;
    • nitpick: may be good to add an empty line before this one so that it is known that the comment doesn't apply to it.

      Or even better, a comment along the lines of:

      Synthetic variable to keep track of whether a return statement has been executed.

    • Yep, this predates those comments. Fixed in v2, thanks.

    • Please register or sign in to reply
  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit 6a1c973f
  • 2401 2401 break;
    2402 2402
    2403 2403 default:
    2404 FIXME("Unhandled instruction type %s.\n", hlsl_node_type_to_string(instr->type));
    2404 hlsl_fixme(ctx, &instr->loc, "Instruction type %s.", hlsl_node_type_to_string(instr->type));
  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 6a1c973f
  • 2927 3126 transform_ir(ctx, find_recursive_calls, body, &recursive_call_ctx);
    2928 3127 vkd3d_free(recursive_call_ctx.backtrace);
    2929 3128
    3129 /* Avoid going into an infinite loop when processing them. */
  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 6a1c973f
  • 541 * statement in this block. We have to stop and do this in a separate
    542 * loop, because instructions must be deleted in reverse order (due to
    543 * def-use chains.)
    544 *
    545 * If we're inside of a loop CF block, we can instead just turn the
    546 * return into a break, which offers the right semantics—except that it
    547 * won't break out of nested loops.
    548 *
    549 * - A CF block which might contain a return statement. After calling
    550 * lower_return() on the CF block body, we stop, pull out everything after
    551 * the CF instruction, shove it into an if block, and then lower that if
    552 * block.
    553 *
    554 * (We could return a "did we make progress" like transform_ir() and run
    555 * this pass multiple times, but we already know the only block that still
    556 * needs addressing, so there's not much point.)
    • Comment on lines +554 to +556

      The previous part of the comment is great at explaining what this does!

      This paragraph got me a little confused at first though, because I didn't made the association with the progress flag immediately. Perhaps a rephrasing would help:

      We could return a "did we make progress" boolean like in transform_ir() and run this pass multiple times, but we already know the only block that remains to be addressed, so there's not much point.

      Also, I have the feeling that this approach wouldn't be easy either, because we would have to check if we already inserted the early_return_var conditionals during the last iteration.

    • Elizabeth Figura changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • The previous part of the comment is great at explaining what this does!

      Thanks, I'm glad to hear that. Explaining the code was probably my biggest concern when writing this; it's not a simple algorithm.

      This paragraph got me a little confused at first though, because I didn't made the association with the progress flag immediately. Perhaps a rephrasing would help:

      We could return a "did we make progress" boolean like in transform_ir() and run this pass multiple times, but we already know the only block that remains to be addressed, so there's not much point.

      Yeah, the original is at least missing a word; I've adjusted it a little.

    • Please register or sign in to reply
  • Excellent MR!

    We have to remember to keep the clone functions in mind from now on if we add new fields to instruction types, but they seem necessary.

  • Elizabeth Figura added 13 commits

    added 13 commits

    • 71a09412...902ddee5 - 5 commits from branch wine:master
    • 9e2128dd - tests: Avoid performing a multi-component UAV load in uav-out-param.shader_test.
    • cd2ba9bf - vkd3d-shader/hlsl: Skip functions that don't have a body when looking for the entry point.
    • 9aa0cee4 - vkd3d-shader/hlsl: Emit a hlsl_fixme() if multiple valid entry point definitions are given.
    • d8e27964 - vkd3d-shader/hlsl: Lower return statements.
    • c031993d - vkd3d-shader/hlsl: Emit a hlsl_fixme() for unhandled instruction types when writing bytecode.
    • 5336fcf0 - vkd3d-shader/hlsl: Inline function calls.
    • 6f8044dd - vkd3d-shader/hlsl: Handle early return after a CF block only if there was...
    • d2fc39a2 - vkd3d-shader/hlsl: Allow the final expression in a for loop initializer to be omitted.

    Compare with previous version

    • If possible, please don't rebase on the new master while we're still doing the review, as it muddies the diff between patch set versions that GitLab creates. Once everything is ironed out, you can rebase on the latest master (or even let AJ do it directly if there are not conflicts).

    • Sorry about that, I thought I hadn't rebased it in the meantime but I was wrong. I'll be more careful about that in the future...

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

    approved this merge request

  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit 5336fcf0
  • 1280 hlsl_src_from_node(&dst->path[i], map_instr(map, src->path[i].node));
    1281
    1282 return true;
    1283 }
    1284
    1285 static void clone_src(struct clone_instr_map *map, struct hlsl_src *dst, const struct hlsl_src *src)
    1286 {
    1287 hlsl_src_from_node(dst, map_instr(map, src->node));
    1288 }
    1289
    1290 static struct hlsl_ir_node *clone_call(struct hlsl_ctx *ctx, struct hlsl_ir_call *src)
    1291 {
    1292 struct hlsl_ir_node *dst;
    1293
    1294 return hlsl_new_call(ctx, src->decl, &src->node.loc);
    1295 }
  • Elizabeth Figura added 5 commits

    added 5 commits

    • a45414bc - vkd3d-shader/hlsl: Lower return statements.
    • 49fa7373 - vkd3d-shader/hlsl: Emit a hlsl_fixme() for unhandled instruction types when writing bytecode.
    • 35152f49 - vkd3d-shader/hlsl: Inline function calls.
    • 60944e5e - vkd3d-shader/hlsl: Handle early return after a CF block only if there was...
    • 375ff9f3 - vkd3d-shader/hlsl: Allow the final expression in a for loop initializer to be omitted.

    Compare with previous version

  • Francisco Casas approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

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