Skip to content
Snippets Groups Projects

hlsl: Define some intrinsics in HLSL.

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

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
  • Author Developer

    We've been adding increasingly complex intrinsics, and after looking at an implementation of inverse trigonometry intrinsics sent to me privately, I've decided that reviewing particularly complex intrinsics written in C is getting unwieldy. At least, it's difficult to actually double-check the math like that.

    So I decided to see how difficult it would be to define these functions in HLSL instead. The main reason we haven't done that historically is because most of these functions are not really expressible in HLSL, mostly by virtue of taking polymorphic type arguments. In this patch series I resolve that by basically generating type variants one at a time when the function is invoked.

    Apart from that the implementation is relatively simple. We reuse the same hlsl_ctx but a new flex scanner, and save and restore a couple of ctx members so that we're in global scope, then we just invoke the function with a HLSL_IR_CALL, the same way we do with user functions.

    In theory, we could reuse this code for complex lowering intrinsics as well—float modulus and integer div/mod come to mind. Currently what makes that awkward is that those lowering passes are done on the entry point after inlining calls, while invoking HLSL-defined functions requires another HLSL_IR_CALL. But this is a relatively simple problem to fix; probably we either run lowering passes earlier, on all functions, or just rerun function inlining.

  • Here are the inverse trig intrinsics Zeb is talking about:

    petrathekat/vkd3d@master...d3d-inverse-trig-55154

  • Francisco Casas
  • Francisco Casas
  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 1beec15c
  • 3468 return false;
    3429 static const char template[] =
    3430 "%s smoothstep_%s(%s low, %s high, %s x)\n"
    3431 "{\n"
    3432 " %s p = saturate((x - low) / (high - low));\n"
    3433 " return (p * p) * (3 - 2 * p);\n"
    3434 "}";
    3469 3435
    3470 if (!(p = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, p, p, loc)))
    3436 if (!(type = elementwise_intrinsic_get_common_type(ctx, params, loc)))
    3471 3437 return false;
    3438 type = hlsl_get_numeric_type(ctx, type->class, HLSL_TYPE_FLOAT, type->dimx, type->dimy);
    3472 3439
    3473 if (!(res = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, p, res, loc)))
    3440 name = hlsl_sprintf_alloc(ctx, "<internal>smoothstep_%s", type->name);
    3441 body = hlsl_sprintf_alloc(ctx, template, type->name, type->name, type->name, type->name, type->name, type->name);
  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit 1beec15c
  • 2994 3004 ERR("Failed to insert function overload.\n");
    2995 3005 vkd3d_free(name);
    2996 3006 return;
    2997 3007 }
    • Comment on lines 2988 to 2997

      Note that we are triggering this ERR if we have multiple calls to the same intrinsic, e.g.:

      float4 main(uniform float4 u) : sv_target
      {
          return lit(u.x, u.y, u.z) + lit(u.x, u.y, u.z);
      }

      because we are declaring the internal multiple times.

      Perhaps hlsl_compile_internal_function() should check if the internal with the given name is already compiled.

    • Please register or sign in to reply
  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 1beec15c
  • 3458 hlsl_block_add_instr(params->instrs, minus_two);
    3459
    3460 if (!(three = hlsl_new_float_constant(ctx, 3.0, loc)))
    3461 return false;
    3462 hlsl_block_add_instr(params->instrs, three);
    3463
    3464 if (!(res = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, minus_two, p, loc)))
    3465 return false;
    3425 struct hlsl_ir_function_decl *func;
    3426 struct hlsl_type *type;
    3427 char *name, *body;
    3466 3428
    3467 if (!(res = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_ADD, three, res, loc)))
    3468 return false;
    3429 static const char template[] =
    3430 "%s smoothstep_%s(%s low, %s high, %s x)\n"
    • Do we need to manually define the internal function with a new name for each overload?

      I see that this is currently the case because hlsl_compile_internal_function() uses hlsl_get_func_decl() internally.

      But, since we are compiling the function once for each call to the intrinsic anyways, what if we change hlsl_compile_internal_function() slightly so that it only expects the return type, the clean name of the function (e.g. just "smoothstep") and the body without the signature?

      This so that only hlsl_compile_internal_function() has to care about the real internal name of the compiled function, maybe using an internal counter to give them a unique name e.g. <internal-42>smoothstep. That way we don't have to care about overloads.

    • I think I would also be in favor of generating a new name each time an internal function is parsed.

    • Author Developer

      Yes, that's nicer. My original plan was to avoid compiling the same function multiple times, but I think a nicer way to do that is to cache them in dedicated hlsl_ctx entries.

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

      changed this line in version 2 of the diff

    • Please register or sign in to reply
  • I think this is a very useful tool. I am ready to approve after my comments have been addressed.

  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Elizabeth Figura added 5 commits

    added 5 commits

    • da1eae21 - vkd3d-shader/hlsl: Introduce an hlsl_sprintf_alloc() helper.
    • a6f1b23c - vkd3d-shader/hlsl: Separate an add_user_call() helper.
    • dcaefa71 - vkd3d-shader/hlsl: Store the internal name counter in struct hlsl_ctx.
    • 1309e10a - vkd3d-shader/hlsl: Define smoothstep() in HLSL.
    • e2e1cf39 - vkd3d-shader/hlsl: Define lit() in HLSL.

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit dcaefa71
  • 1060 1060 {
    1061 1061 struct vkd3d_string_buffer *string;
    1062 1062 struct hlsl_ir_var *var;
    1063 static LONG counter;
    1064 1063
    1065 1064 if (!(string = hlsl_get_string_buffer(ctx)))
    1066 1065 return NULL;
    1067 vkd3d_string_buffer_printf(string, "<%s-%u>", template, InterlockedIncrement(&counter));
    1066 vkd3d_string_buffer_printf(string, "<%s-%u>", template, ctx->internal_name_counter++);
    • This made me notice that we don't verify the return value for vkd3d_string_buffer_printf(). And apparently it's endemic in vkd3d-shader, so I can't really blame it on you, but maybe it makes sense to start rectifying it? The same happens in 4/5.

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

    approved this merge request

    • This made me notice that we don't verify the return value for vkd3d_string_buffer_printf(). And apparently it's endemic in vkd3d-shader, so I can't really blame it on you, but maybe it makes sense to start rectifying it?

      Mostly because it's not terribly practical to check in anything that outputs more than one or two lines. I think we should adopt the vkd3d_bytecode_buffer scheme and track the status on the vkd3d_string_buffer instead of attempting to propagate errors up from vkd3d_string_buffer_resize().

      POSIX allows to use the %m$ syntax and avoid repeat many times the same argument, but unfortunately the standard printf() doesn't...

      Sadly, it seems that MinGW in ANSI mode doesn't implement this feature, otherwise I'd be glad to depend on it anyway.

      Could we perhaps use _vsprintf_p() on the only non-POSIX platform we care about?

    • Mostly because it's not terribly practical to check in anything that outputs more than one or two lines. I think we should adopt the vkd3d_bytecode_buffer scheme and track the status on the vkd3d_string_buffer instead of attempting to propagate errors up from vkd3d_string_buffer_resize().

      Agreed. What I don't like is that no check of allocation failure is done at all.

    • Please register or sign in to reply
  • Henri Verbeet approved this merge request

    approved this merge request

  • Francisco Casas 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