vkd3d-shader/hlsl: Parse and write default values.
Merge request reports
Activity
added 6 commits
- 35856c4a - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- abca0b97 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- 3b080b79 - vkd3d-shader/tpf: Write default values.
- c6d0175d - tests: Text matrix default value initializers.
- 5af7d197 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- 5f638855 - vkd3d-shader/hlsl: Reorder default values for matrices.
Toggle commit listWhy lower them after parsing? Can't we reuse (most of) evaluate_static_expression_as_uint()?
It is a little more complicated than when evaluating a single uint because it requires breaking the rhs values into separate components (which can be of different types) and it also must have access to the value of static constants.
For instance:
static const float4 a = float4(1, 2, 3, 4); int4 b = {1, float2(2, a.z), a.w};
so the same instruction block must be shared between components and must include the static_initializers instructions.
I could call constant-folding and copy-propagation early at parse time over ctx->static_initializers for every variable to lower the components to constants as soon as possible but that doesn't seem very clean to me.
Lowering the default values after the regular constant-folding and copy-prop passes allows for the simple implementation of lower_var_default_values() in the current version.
sm1 has default values as well; if we're not going to write them we should at least have a hlsl_fixme().
Oh! I didn't realize about this because they don't show up in native's disassembler. Okay, I added it in 7/10.
static const float4 a = float4(1, 2, 3, 4); int4 b = {1, float2(2, a.z), a.w};
so the same instruction block must be shared between components and must include the static_initializers instructions.
Sorry, I don't understand? Why does this mean we need the same instruction block?
We should just be able to clone it, prepend the cloned static_initializers the same way that evaluate_static_expression_as_uint() already does, and append a hlsl_new_load_component() as necessary.
added 8 commits
- b640bfe7 - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- 05548da2 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- c3eaa652 - tests: Test default values using reflection.
- fb8e16f8 - vkd3d-shader/tpf: Write default values for SM4.
- 20609d2b - vkd3d-shader/d3dbc: Write default values for SM1.
- e12e7214 - tests: Text matrix default value initializers.
- faaf5074 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- 384ec54f - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit listCould you please add hlsl_fixme()'s to write_fx_2_initial_value() and write_fx_4_numeric_variable() when default values are present.
done!
Another thing I wanted to mention, where would we put a value pointer for string variables?
My guess is that it would make sense to add a
const char *
field tostruct hlsl_default_value
. Because of this suggestion I didn't turnhlsl_ir_var.default_values
into a mereunion hlsl_constant_value_component
, because in the future we might want to add extra stuff such as this.Another thing that will be needed is CompileShader/ConstructGSWithSO/compile settings, it's at most 3 strings I think, or 2 strings and a function prototype. We'll probably need small structure for that in hlsl_default_value as well. When used without variable it still creates unnamed object in effects binary, so it's not the same as other state assignments (assignment will be created too, but it will assign that unnamed object).
added 14 commits
- 384ec54f...afadfe5a - 4 earlier commits
- 3a17bf27 - tests: Test complex array size expression.
- 59c18786 - vkd3d-shader/hlsl: Also lower matrix swizzles and index loads in const passes.
- c5cfb53e - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- 582db423 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- e6fad279 - tests: Test default values using reflection.
- d7ef580a - vkd3d-shader/tpf: Write default values for SM4.
- b3593ac8 - vkd3d-shader/d3dbc: Write default values for SM1.
- 9e10606e - tests: Text matrix default value initializers.
- 86afe650 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- 3434cb48 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit listNew version, now computing final default values at parse time as suggested by Zeb.
I think this is good since we no longer require an
hlsl_src
instruct hlsl_default_value
, but it is a little more complex:- Requires more passes on constant evaluation, some of these are useful for array sizes though!
- Requires allowing stores to synthetic variables, which can arise when storing/loading from components, specially if there are value constructors such as
float2x3(1,2,3,4,5,6)
in the default value initializers. - There is more overhead but I doubt this will be a concern.
I know the MR is chonky now, if this is a problem, I can send the first 6 patches as a separate MR.
mentioned in merge request !839 (merged)
From 7/14:
I thought I said this somewhere, but I don't think hlsl_transfer_var_default_values() needs to be an entire helper, nor does it need to be in hlsl.c. I'd just write it inline. Same with hlsl_init_var_default_values().
+ case HLSL_IR_STORE: + if (hlsl_ir_store(node)->lhs.var->is_synthetic) + break; + /* fall-through */
That's odd. Why are there stores left after DCE?
"save_default_values" feels a bit odd to me naming-wise, and moreover, why do we need that in two separate blocks? Also, why "empty" the instrs block? [Is that left over from the initial revision?]
The reflection tests (9/14) are a bit odd in that they test things unrelated to default values, but don't quite integrate with test_reflection() either. I'd just pick one or the other...
Is 12/14 offering any benefit that's not already in the reflection tests?
14/14 seems like the wrong way to solve this, and the explanation is a bit awkward—default values aren't special either, they're just in actual byte order, which is different from component order. I'd think the right thing to do would be to implement that mapping in hlsl_type_get_component_offset() [which does require handling matrices specially, yes.]
I thought I said this somewhere, but I don't think hlsl_transfer_var_default_values() needs to be an entire helper, nor does it need to be in hlsl.c. I'd just write it inline. Same with hlsl_init_var_default_values().
done.
That's odd. Why are there stores left after DCE?
I am not sure I understand the question. hlsl_run_const_passes() is not called yet and it does not run DCE (?)
I had to allow stores to synthetic variables because they are needed by complex initializers when there are multi-value parse-time arguments in the initializer, such as constructors or matrix indexing:
static const float3x2 m = {1, 2, 3, 4, 5, 6}; float4 a = {float2(1, 2), m[2]};
"save_default_values" feels a bit odd to me naming-wise, and moreover, why do we need that in two separate blocks?
I changed it to
is_default_values_initializer
. It is better?Ehm, the first block checks if the variable needs to allocate the
var->default_values
array. The initializer logic in-between the blocks is required whether we need to store the default values or not. And at the end there comes 3 cases from whichsave_default_values
is one of them, to decide what to do withv->initializer.instrs
. But you probably already know that, so maybe you are seeing something that I don't see yet.The reflection tests (9/14) are a bit odd in that they test things unrelated to default values, but don't quite integrate with test_reflection() either. I'd just pick one or the other...
Okay, I removed the checks for
BoundResources
,buffer_desc.Name
,buffer_desc.Size
,buffer_desc.uFlags
,var_desc.Name
,var_desc.StartOffset
,var_desc.uFlags
,var_desc.StartTexture
,var_desc.TextureSize
,var_desc.StartSampler
, andvar_desc.SamplerSize
.Also, why "empty" the instrs block? [Is that left over from the initial revision?]
Oh, I see now that the call to
free_parse_variable_def(v)
later takes care of it, yep, so it is not necessary to clean it.Is 12/14 offering any benefit that's not already in the reflection tests?
I feel that it is good for the completeness of the test file, as a sort of documentation of this anomaly. Also, I think it is good to have tests that run infrequent code paths more in isolation, which can be useful e.g. if valgrind or other tool detects an error there.
14/14 seems like the wrong way to solve this, and the explanation is a bit awkward—default values aren't special either, they're just in actual byte order, which is different from component order. I'd think the right thing to do would be to implement that mapping in hlsl_type_get_component_offset() [which does require handling matrices specially, yes.]
I am not sure if it is correct to say that they end up in actual byte order, because that is not the case for row_major matrices in SM4:
row_major int2x3 m = {1, 2, 3, 4, 5, 6}; // row_major int2x3 m; // Offset: 0 Size: 28 // = 0x00000001 0x00000003 0x00000005 0x00000000 // 0x00000002 0x00000004 0x00000006 float4 main() : sv_target { return m[0][0]; }
unless we call it "byte order if the matrix was column-major".
The way I see it, there are 3 different orders involved:
- The order of the initializer arguments (A).
- The HLSL component indexes (B).
- The offsets in the default values written in the output binary (C), d3dbc or tpf.
Here is how default values are stored for a column-major and a row-major matrix both in d3dbc and tpf. The possition of the padding words (zeroes) suggests how the values ended up in the HLSL matrix.
float2x3 m = {1, 2, 3, 4, 5, 6}; stored in d3dbc: 1 4 0 0 2 5 0 0 3 6 0 0 suggests: [ 1 2 3 ] [ 4 5 6 ] stored in tpf: 1 2 0 0 3 4 0 0 5 6 suggests: [ 1 3 5 ] [ 2 4 6 ]
row_major float2x3 m = {1, 2, 3, 4, 5, 6} stored in d3dbc: 1 2 3 0 4 5 6 0 suggests: [ 1 2 3 ] [ 4 5 6 ] stored in tpf: 1 3 5 0 2 4 6 suggests: [ 1 3 5 ] [ 2 4 6 ]
Given the values for the HLSL components, there is a transformation from A -> B that is not trivial at least for SM4.
My current implementation uses hlsl_reorder_default_values_in_matrices() to perform this A -> B transformation and then the same hlsl_type_get_component_offset() we use for the rest of HLSL to do B -> C.
If I understand correctly, you propose something along the lines of storing the default values as they come in the initializer in
var->default_values[]
(order A) and then A -> B -> C in hlsl_type_get_component_offset(), probably adding abool default_values
argument.This feels odd to me because the
index
argument would no longer mean "component index in the HLSL sense", but rather "component index in the initializer sense" and we would lost the possibility of directly taking the default value of an HLSL component by accessingvar->default_values[]
using the component index, if we ever need it.But to be completely sure that the anomaly is indeed in the A -> B part, and that is not that the default values for matrices are just stored in an apparently nonsensical way, I will check how ID3DX11EffectPass::Apply() takes this data and uses it to automatically write the constant buffer.
That's odd. Why are there stores left after DCE?
I am not sure I understand the question. hlsl_run_const_passes() is not called yet and it does not run DCE (?)
Oops, I keep assuming that DCE is always run.
Thinking about it a bit more though I guess we don't want it here... since we're probably supposed to reject stores to "unrelated" variables. Annoying but I guess this is probably better.
"save_default_values" feels a bit odd to me naming-wise, and moreover, why do we need that in two separate blocks?
I changed it to
is_default_values_initializer
. It is better?Ehm, the first block checks if the variable needs to allocate the
var->default_values
array. The initializer logic in-between the blocks is required whether we need to store the default values or not. And at the end there comes 3 cases from whichsave_default_values
is one of them, to decide what to do withv->initializer.instrs
. But you probably already know that, so maybe you are seeing something that I don't see yet.No, I'm just blind, sorry.
Is 12/14 offering any benefit that's not already in the reflection tests?
I feel that it is good for the completeness of the test file, as a sort of documentation of this anomaly. Also, I think it is good to have tests that run infrequent code paths more in isolation, which can be useful e.g. if valgrind or other tool detects an error there.
Why not put that documentation in the reflection tests, though?
I'm not sure I understand the isolation comment. If necessary one can run the reflection tests and comment out all the other tests in that file?
14/14 seems like the wrong way to solve this, and the explanation is a bit awkward—default values aren't special either, they're just in actual byte order, which is different from component order. I'd think the right thing to do would be to implement that mapping in hlsl_type_get_component_offset() [which does require handling matrices specially, yes.]
I am not sure if it is correct to say that they end up in actual byte order, because that is not the case for row_major matrices in SM4:
Ah, you're right. d3dx9 does explicitly transpose, and presumably d3dx10 does too...
Still, fundamentally this is a translation problem. Using a "pass", especially to perturb the array in place, seems wrong.
If I understand correctly, you propose something along the lines of storing the default values as they come in the initializer in
var->default_values[]
(order A) and then A -> B -> C in hlsl_type_get_component_offset(), probably adding abool default_values
argument.This feels odd to me because the
index
argument would no longer mean "component index in the HLSL sense", but rather "component index in the initializer sense" and we would lost the possibility of directly taking the default value of an HLSL component by accessingvar->default_values[]
using the component index, if we ever need it.Not necessarily, we should store it in whatever order is most convenient (but ideally a backend-neutral one, so not the actual register order). Fundamentally we don't have to use hlsl_type_get_component_offset() per se if that doesn't work, but we should be doing the translation in the same place as we were / would be calling that.
Why not put that documentation in the reflection tests, though?
Reading the individual tests can be more readable than these tests that check for many things at the same time.
I'm not sure I understand the isolation comment. If necessary one can run the reflection tests and comment out all the other tests in that file?
I meant that this test is even smaller than the reflection test so the possible causes of the error are more narrow. But if you want I can drop the patch.
Ah, you're right. d3dx9 does explicitly transpose, and presumably d3dx10 does too...
Yep, note that changing the reading order is not equivalent to transposing though.
[ 1 2 3 ] -> [ 1 3 5 ] [ 4 5 6 ] [ 2 4 6 ]
Still, fundamentally this is a translation problem. Using a "pass", especially to perturb the array in place, seems wrong.
Okay. Now the question that remains is whether the translation problem is from the initializer to the HLSL matrix, or from the HLSL matrix to the default values. I think it is the former case given that the output tpf suggest a structure for the matrix.
I replaced
hlsl_reorder_default_values_in_matrices()
with theget_component_index_from_default_initializer_index()
, which transforms the index from initializer index to component index.
added 157 commits
-
3434cb48...766913f9 - 143 commits from branch
wine:master
- 766913f9...31738cce - 4 earlier commits
- f9ac97c3 - tests: Test complex array size expression.
- 9faf2843 - vkd3d-shader/hlsl: Also lower matrix swizzles and index loads in const passes.
- 17e3b5ca - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- cd61a4cc - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- fbc10729 - tests: Test default values using reflection.
- 2a1d011c - vkd3d-shader/tpf: Write default values for SM4.
- 1d03b543 - vkd3d-shader/d3dbc: Write default values for SM1.
- cd33d390 - tests: Test matrix default value initializers.
- a2414e2f - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- 97ca87aa - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit list-
3434cb48...766913f9 - 143 commits from branch
added 72 commits
-
97ca87aa...4b3a948e - 58 commits from branch
wine:master
- 4b3a948e...f0361e71 - 4 earlier commits
- 71a8bd43 - tests: Test complex array size expression.
- 871b4bec - vkd3d-shader/hlsl: Also lower matrix swizzles and index loads in const passes.
- 6e3ae88d - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- 45ede20e - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- 337afa95 - tests: Test default values using reflection.
- 44412af5 - vkd3d-shader/tpf: Write default values for SM4.
- 856b036b - vkd3d-shader/d3dbc: Write default values for SM1.
- 17548ada - tests: Test matrix default value initializers.
- 6ea2cb15 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- c8088891 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit list-
97ca87aa...4b3a948e - 58 commits from branch
added 1 commit
- 4a096c2c - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Thanks! Can you approve !839 (merged) first so that those first patches get upstreamed?
added 55 commits
-
4a096c2c...061dc390 - 47 commits from branch
wine:master
- 24869521 - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- 5b5cf657 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- 060bf3d4 - tests: Test default values using reflection.
- 466ca28c - vkd3d-shader/tpf: Write default values for SM4.
- 2e4f61a8 - vkd3d-shader/d3dbc: Write default values for SM1.
- bd726113 - tests: Test matrix default value initializers.
- a78f9399 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- 9173fe34 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit list-
4a096c2c...061dc390 - 47 commits from branch
added 16 commits
-
9173fe34...9c83caed - 8 commits from branch
wine:master
- 94bfd90c - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- 798fbd3d - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- 91d55f34 - tests: Test default values using reflection.
- 9c46f69e - vkd3d-shader/tpf: Write default values for SM4.
- 91fc5e1f - vkd3d-shader/d3dbc: Write default values for SM1.
- 6373cdc4 - tests: Test matrix default value initializers.
- e1255ccb - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- 3e9196bc - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit list-
9173fe34...9c83caed - 8 commits from branch
added 38 commits
-
3e9196bc...1fe7a658 - 30 commits from branch
wine:master
- e8dbc36b - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
- 099a64ae - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
- b44a25ce - tests: Test default values using reflection.
- 253c9941 - vkd3d-shader/tpf: Write default values for SM4.
- affadf31 - vkd3d-shader/d3dbc: Write default values for SM1.
- ade3daa3 - tests: Test matrix default value initializers.
- ab01fedc - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
- f5bfa728 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.
Toggle commit list-
3e9196bc...1fe7a658 - 30 commits from branch