Re: [PATCH vkd3d v2] vkd3d-shader/hlsl: Add cross() intrinsic function and test.

[Zebediah Figura (she/her)]

On 11/17/21 08:12, Francisco Casas wrote:
> +static bool intrinsic_cross(struct hlsl_ctx *ctx,
> -
>    const struct parse_initializer *params, struct vkd3d_shader_location loc)
>
> +{
> - struct hlsl_ir_swizzle *arg1_swzl1, *arg1_swzl2, *arg2_swzl1, *arg2_swzl2;
> - struct hlsl_ir_node *arg1 = params->args[0], *arg2 = params->args[1];
> - struct hlsl_ir_node *arg1_cast, *arg2_cast, *mul1_neg;
> - struct hlsl_ir_expr *mul1, *mul2;
> - struct hlsl_type *cast_type;
> - enum hlsl_base_type base;
> - int wrong_arg = -1;
> -
> - if (arg2->data_type->type != HLSL_CLASS_VECTOR)
> -
>    wrong_arg = 1;
> - if(arg2->data_type->dimx != 3 && arg2->data_type->dimx != 4)

Or, more simply, "dimx < 3".

This doesn't seem to be quite right, though. It's legal to pass a scalar 
to these functions. I suspect that what should instead happen is that we 
should try to implicitly convert to float3 or half3, and let that part 
fail (and generate the appropriate error message).

(On the other hand, this wrongly allows matrices, but HLSL function 
matching logic is arcane, and for the sake of simplicity I'd rather just 
be more lenient than native.)
> -
>    wrong_arg = 1;
> -
> - if (arg1->data_type->type != HLSL_CLASS_VECTOR)
> -
>    wrong_arg = 0;
> - if (arg1->data_type->dimx != 3 && arg1->data_type->dimx != 4)
> -
>    wrong_arg = 0;
> -
> - if (wrong_arg != -1)
> - {
> -
>    struct vkd3d_string_buffer *string;
> -
>    if ((string = hlsl_type_to_string(ctx, params->args[wrong_arg]->data_type)))
> -
>        hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
> -
>                "Wrong type for argument %d of 'cross': '%s'.", wrong_arg, string->buffer);
> -
>    hlsl_release_string_buffer(ctx, string);
> -
>    return false;
> - }

There's no reason we should only generate one error here; we should 
generate errors for each argument separately.
> -
> - if (arg1->data_type->base_type == HLSL_TYPE_HALF && arg2->data_type->base_type == HLSL_TYPE_HALF){
> -
>    base = HLSL_TYPE_HALF;
> - } else {
> -
>    base = HLSL_TYPE_FLOAT;
> - }
> - base = expr_common_base_type(base, arg1->data_type->base_type);
> - base = expr_common_base_type(base, arg2->data_type->base_type);

These expr_common_base_type() calls are basically redundant.