tests: Add more function overloading tests.
Merge request reports
Activity
We need tests for:
-
multiple arguments [and this is the main thing that makes prioritization very hard];
-
widening casts;
-
1xN/Nx1 matrix special cases covered in implicit_compatible_data_types();
-
1x4/2x2 matrices;
-
arrays. Note also that what's considered a legal implicit cast for structs is actually just whether the type has the same components in the same order, not the same fields. I.e. see cast-componentwise-equal.shader_test.
+% structs do not prioritize naming
I don't see how this test is proving that, since you're passing an "a" and the available overloads take "b" and "c".
-
added 894 commits
-
6c4a9f16...9cb4207c - 892 commits from branch
wine:master
- 4760943a - tests: Add more function overloading tests.
- a085fc89 - vkd3d-shader/hlsl: Prioritize between compatible function overloads.
-
6c4a9f16...9cb4207c - 892 commits from branch
There's a few instances of "cast" which I guess are intended to filter out anything that would be a cast in the bytecode (with sm4-colored glasses, but you know how it is...)? Maybe "noop_cast" with inverted meaning would be better?
What does type_is_cast() look like once we have more complete minimum-precision type support? I'm assuming that holds, so it allows min16uint <-> min16int, and nothing else?
I'm struggling to understand get_overload_element_check_count(). If it can be clarified a bit, that's great, but since it's imported code I don't care greatly.
> +#define DIST_COND(shift, cond) \ > + do { \ > + if (cond) \ > + { \ > + distance += 1ull << (2 + 7 * shift); \ > + } \ > + } while(0) \ > + > + DIST_COND(0, arg_components < param_components); > + DIST_COND(1, param_promotion); > + DIST_COND(2, arg_promotion); > + DIST_COND(3, !param_exact_match); > + DIST_COND(4, !arg_exact_match); > + DIST_COND(5, param_cast); > + DIST_COND(6, arg_cast); > + DIST_COND(7, param_components < arg_components); > +#undef DIST_COND
This macro doesn't seem like it's actually doing any work? Open-coding would work just as well.
> + if (ambiguous) > + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_AMBIGUOUS_FUNCTION_CALL, "Ambiguous function call.");
I won't insist on it for this merge request, but it would be nice to report the candidates here.
There's a few instances of "cast" which I guess are intended to filter out anything that would be a cast in the bytecode (with sm4-colored glasses, but you know how it is...)? Maybe "noop_cast" with inverted meaning would be better?
I didn't quite understand what you're asking and suggesting here, can you clarify?
What does type_is_cast() look like once we have more complete minimum-precision type support? I'm assuming that holds, so it allows min16uint <-> min16int, and nothing else?
Surprisingly, no, or at least not in dxc. There are even comments stating that the minimum precision types are only compatible among themselves. dxc considers the new SM6 fixed width ints and uints as not casts, but other than that it also has a concept of literal types (i.e literal int and literal float) and considers conversions between those and concrete types as not casts.
I'm struggling to understand get_overload_element_check_count(). If it can be clarified a bit, that's great, but since it's imported code I don't care greatly.
Suppose we're calling a function with these overloads:
float4 f(float2 x[12]) { return 2.0; } float4 f(float3 x[8]) { return 3.0; } float4 main() : sv_target { float4 x[6]; return f(x); }
Normally we'd compare all 24 components, but that function reduces comparisons like the one between
float4[6]
andfloat2[12]
to just 4 components. It's arguably a very niche thing, but dxc implements it so I just ported it over.DIST_COND
Ditched.
Candidate reporting
Done!
There's a few instances of "cast" which I guess are intended to filter out anything that would be a cast in the bytecode (with sm4-colored glasses, but you know how it is...)? Maybe "noop_cast" with inverted meaning would be better?
I didn't quite understand what you're asking and suggesting here, can you clarify?
type_is_cast() (and a couple places in the tests?) use "cast" to apparently mean "anything that might change the byte representation". [In theory this would also exclude e.g. float/half conversion, but apparently it doesn't.]
Generally "cast" means any explicit or implicit cast from one type to another, so this use of "cast" is a bit misleading. I'm suggesting to rename type_is_cast() to type_is_noop_cast() and invert its meaning.
The tests also include the sentence "Casting the parameter's type ("param_cast") is preferred over casting the argument's type.", which is confusing. Reading through type_overload_distance() I kind of understand what it's saying, and I'm not really sure how to better phrase that, except I guess to explicitly explain something like:
"Let C be the common type that would be produced in an arithmetic expression between the parameter and argument types. If C is the same as the argument, the overload is preferred to the case where C is the same as the parameter but different to the argument. E.g. in the following example, C is "int" for the first function (same as the argument) and "float" for the second function (same as the parameter), so the first function is preferred."
What does type_is_cast() look like once we have more complete minimum-precision type support? I'm assuming that holds, so it allows min16uint <-> min16int, and nothing else?
Surprisingly, no, or at least not in dxc. There are even comments stating that the minimum precision types are only compatible among themselves.
So it's only uint and int that are considered equal? In that case maybe the switch is overkill?
dxc considers the new SM6 fixed width ints and uints as not casts, but other than that it also has a concept of literal types (i.e literal int and literal float) and considers conversions between those and concrete types as not casts.
Ah, I believe this is also an sm1/sm4 phenomenon. I'd been referring to these as "ambiguous int" but I suppose "literal" is a better name. This is behaviour we should add eventually; it's on my list while I continue to refactor the type system.
Normally we'd compare all 24 components, but that function reduces comparisons like the one between
float4[6]
andfloat2[12]
to just 4 components. It's arguably a very niche thing, but dxc implements it so I just ported it over.Ah I see, so it's just an optimization, right? Since it's basically an optimization for hlsl_types_are_componentwise_{compatible,equal}() maybe it should be implemented there instead?
type_is_cast() (and a couple places in the tests?) use "cast" to apparently mean "anything that might change the byte representation".
I think I understand your point, but the minimum precision ints are also casts between themselves, so I think the use of the word "cast" here is just an arbitrary choice. I don't really like inverting the function though, because we'd return a negated boolean and we'd have to negate it again in
type_overload_distance
."Let C be the common type [...]"
That's a much better explanation, would you prefer if I added that to the test file?
So it's only uint and int that are considered equal? In that case maybe the switch is overkill?
For our current implementation of the HLSL type system, yes.
Since it's basically an optimization for hlsl_types_are_componentwise_{compatible,equal}() maybe it should be implemented there instead?
Maybe, but we probably want it here as well, otherwise we'd compare more elements than we need to in the
i < compare_elements
loop. (arguably not a big deal, except for some ridiculously large arrays)type_is_cast() (and a couple places in the tests?) use "cast" to apparently mean "anything that might change the byte representation".
I think I understand your point, but the minimum precision ints are also casts between themselves, so I think the use of the word "cast" here is just an arbitrary choice. I don't really like inverting the function though, because we'd return a negated boolean and we'd have to negate it again in
type_overload_distance
.I guess I don't see a problem with this. Mentally I find it just as easy to understand a boolean as its negated version. Maybe it's a subjective thing?
The thing is I'd rather see an arbitrary and incorrect term, over an arbitrary and incorrect term that also is used to mean something else in the code.
Though honestly giving it a bit more thought I guess I don't hate type_is_cast(). It's just as almost-true as type_is_noop_cast() would be.
"Let C be the common type [...]"
That's a much better explanation, would you prefer if I added that to the test file?
Sure.
So it's only uint and int that are considered equal? In that case maybe the switch is overkill?
For our current implementation of the HLSL type system, yes.
Well, for any implementation, I would imagine?
Especially since I think we'll eventually have twice as many base types. We want one for each minimum-precision type, plus the "literal int" and "literal float" types. We may also want to split the "double" type based on shader model, since it means different things...
Since it's basically an optimization for hlsl_types_are_componentwise_{compatible,equal}() maybe it should be implemented there instead?
Maybe, but we probably want it here as well, otherwise we'd compare more elements than we need to in the
i < compare_elements
loop. (arguably not a big deal, except for some ridiculously large arrays)Right. I forgot how we're using it so I shouldn't have said "instead".
added 142 commits
-
a085fc89...8a3fe9cd - 140 commits from branch
wine:master
- c296983e - tests: Add more function overloading tests.
- 52ceebaf - vkd3d-shader/hlsl: Prioritize between compatible function overloads.
-
a085fc89...8a3fe9cd - 140 commits from branch
3035 } 3036 3037 static unsigned int get_overload_element_check_count(struct hlsl_type *arg, unsigned int arg_components, 3038 struct hlsl_type *param, unsigned int param_components) 3039 { 3040 unsigned int arg_element_components, param_element_components; 3041 struct hlsl_type *arg_element_type, *param_element_type; 3042 3043 if (arg->class != HLSL_CLASS_ARRAY || param->class != HLSL_CLASS_ARRAY) 3044 return arg_components; 3045 3046 /* As arrays are homogenous in HLSL, we don't need to compare all components 3047 * just enough of them that we know whether they are compatible or not. */ 3048 3049 arg_element_type = arg->e.array.type; 3050 param_element_type = param->e.array.type; changed this line in version 6 of the diff
3078 bool arg_promotion = false, arg_cast = false, arg_exact_match = true; 3079 unsigned int param_components, arg_components, compare_elements; 3080 uint64_t distance = 0; 3081 3082 param_components = hlsl_type_component_count(param); 3083 arg_components = hlsl_type_component_count(arg); 3084 3085 compare_elements = get_overload_element_check_count(arg, arg_components, param, param_components); 3086 compare_elements = min(compare_elements, param_components); 3087 3088 for (unsigned int i = 0; i < compare_elements; ++i) 3089 { 3090 struct hlsl_type *param_comp_type = hlsl_type_get_component_type(ctx, param, i); 3091 struct hlsl_type *arg_comp_type = hlsl_type_get_component_type(ctx, arg, i); 3092 3093 if (param_comp_type->class > HLSL_CLASS_ARRAY || arg_comp_type->class > HLSL_CLASS_ARRAY) Checking
class > HLSL_CLASS_ARRAY
seems brittle to me. From the definition ofenum hlsl_type_class
it is implied that we can compare withHLSL_CLASS_LAST_NUMERIC
to check for numeric types, but the ordering of the following enum values is somewhat arbitrary, perhaps you should check explicitly for numeric, array, or struct, with a helper function.changed this line in version 6 of the diff
I've changed this to a
!= HLSL_CLASS_SCALAR
, which communicates the intent more clearly. While previously this check would also allowHLSL_CLASS_ARRAY
,HLSL_CLASS_STRUCT
,HLSL_CLASS_VECTOR
andHLSL_CLASS_MATRIX
, in practice this could never happen ashlsl_type_get_component_type
will never return a component with those classes.
I am not legal expert so don't weight my opinion too much here, and I would wait for @hverbeet's before changing anything in this regard, but...
I think that among the first paragraphs of the README.md it should be specified that LICENSE is the main license of the vkd3d project and LICENSE_DXC is the DXC's license and is an attachment that concerns a very specific chunck of code in libs/vkd3d-shader/hlsl.y, similarly to how the
Copyrights and Licenses for Third Party Software Distributed with LLVM
section of the LICENSE_DXC does it for DXC.
While it might be tedious, it perhaps even makes sense to have this derived code on its own .h and .c files, for the sake of better delimiting the scope to which the LICENSE_DXC applies, similarly to libs/vkd3d-shader/checksum.c, which contains a note crediting the original author.
Edited by Francisco Casasadded 87 commits
-
52ceebaf...ad2208b7 - 84 commits from branch
wine:master
- 5686d397 - tests: Add more function overloading tests.
- 8ebeec59 - vkd3d-shader/hlsl: Introduce minimum_type_element_comparisons.
- 73577c31 - vkd3d-shader/hlsl: Prioritize between compatible function overloads.
Toggle commit list-
52ceebaf...ad2208b7 - 84 commits from branch
Can we do this without using the DirectXCompiler code? Overload resolution is a fairly well understood problem, and just implementing something fairly standard from scratch should get us close enough, I think?
It depends how accurate we want to be; I don't really see an obvious line to draw. What's wrong with using the DirectXShaderCompiler code?
added 84 commits
-
73577c31...5c00766e - 83 commits from branch
wine:master
- 7b2b5557 - tests: Add more function overloading tests.
-
73577c31...5c00766e - 83 commits from branch