hlsl: Define some intrinsics in HLSL.
Merge request reports
Activity
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:
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
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); changed this line in version 2 of the diff
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.
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.changed this line in version 2 of the diff
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
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.
Toggle commit list1060 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?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 standardprintf()
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.