Re: [PATCH 8/9] d3dx9: Add test for 'cmp' preshader opcode.
2017-05-08 17:39 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/tests/effect.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index 015b505..dfc3f5b 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -4490,7 +4490,8 @@ static void test_preshader_op(IDirect3DDevice9 *device, const DWORD *sample_effe { {0, 0, 0}, {5549, 0, 4}, - {5400, 2, 1} + {5400, 2, 1}, + {4121, 0, 1} };
You could add a ',' after the new array element so that, if we ever need to add another test, that line doesn't need to be changed. Just mentioning it since you have to resend anyway.
DWORD *test_effect_blob; HRESULT hr; @@ -4506,8 +4507,30 @@ static void test_preshader_op(IDirect3DDevice9 *device, const DWORD *sample_effe
test_effect_blob = HeapAlloc(GetProcessHeap(), 0, sample_effect_blob_size); memcpy(test_effect_blob, sample_effect_blob, sample_effect_blob_size); - for (i = 0; i < blob_position[test->args_count].ins_count; ++i) - test_effect_blob[op_pos + i * op_step] = test->opcode; + + if (test->args_count == 3) + { + DWORD *pos; + + pos = &test_effect_blob[op_pos - 1]; + *pos++ = 1; + *pos++ = test->opcode; + *pos++ = test->args_count; + for (i = 0; i < 3; ++i) + { + *pos++ = 0; + *pos++ = 2; + *pos++ = i == 2 ? 4 : 0; + } + *pos++ = 0; + *pos++ = 4; + *pos++ = 0; + } + else + { + for (i = 0; i < blob_position[test->args_count].ins_count; ++i) + test_effect_blob[op_pos + i * op_step] = test->opcode; + }
I think this is getting more and more ugly. What about writing an effect with those 3 preshaders by hand instead? You could start with those edited effect fragments, fixing them up and putting them together in a new effect blob. Sprinkle over some comments as necessary. I'm not going to block the patch for this reason but I think it's something worth exploring.
T On 05/11/2017 05:43 PM, Matteo Bruni wrote:
DWORD *test_effect_blob;
HRESULT hr; @@ -4506,8 +4507,30 @@ static void test_preshader_op(IDirect3DDevice9 *device, const DWORD *sample_effe
test_effect_blob = HeapAlloc(GetProcessHeap(), 0, sample_effect_blob_size); memcpy(test_effect_blob, sample_effect_blob, sample_effect_blob_size); - for (i = 0; i < blob_position[test->args_count].ins_count; ++i) - test_effect_blob[op_pos + i * op_step] = test->opcode; + + if (test->args_count == 3) + { + DWORD *pos; + + pos = &test_effect_blob[op_pos - 1]; + *pos++ = 1; + *pos++ = test->opcode; + *pos++ = test->args_count; + for (i = 0; i < 3; ++i) + { + *pos++ = 0; + *pos++ = 2; + *pos++ = i == 2 ? 4 : 0; + } + *pos++ = 0; + *pos++ = 4; + *pos++ = 0; + } + else + { + for (i = 0; i < blob_position[test->args_count].ins_count; ++i) + test_effect_blob[op_pos + i * op_step] = test->opcode; + }
I think this is getting more and more ugly. What about writing an effect with those 3 preshaders by hand instead? You could start with those edited effect fragments, fixing them up and putting them together in a new effect blob. Sprinkle over some comments as necessary.
I'm not going to block the patch for this reason but I think it's something worth exploring.
The problem is I met this opcode in the wild and thus added it in the first version of preshader implementation (and can "model" and test it by manually editing preshader code), but could not make native d3dx compiler to generate it so far. Whatever I tried in HLSL which would be a straightforward use of 'cmp' resulted in different opcode sequences. It was either generated by some earlier versions of compiler or I just could not find the conditions under which it will be used. As for adding some other 3-parameter opcode to effect blob, while scanning all the opcodes in the 3 hex digit opcode range I have found just one other 3 parameter opcode 'movc', but it seem to always return 0 and I cannot generate it in effect blob from source either. So by now I didn't find any other way to test this opcode rather than manually code it in the effect blob. Maybe I would rather generate the code for all the other cases too? It won't make this part less evident but it will be a uniform approach.
2017-05-11 17:08 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
The problem is I met this opcode in the wild and thus added it in the first version of preshader implementation (and can "model" and test it by manually editing preshader code), but could not make native d3dx compiler to generate it so far. Whatever I tried in HLSL which would be a straightforward use of 'cmp' resulted in different opcode sequences. It was either generated by some earlier versions of compiler or I just could not find the conditions under which it will be used. As for adding some other 3-parameter opcode to effect blob, while scanning all the opcodes in the 3 hex digit opcode range I have found just one other 3 parameter opcode 'movc', but it seem to always return 0 and I cannot generate it in effect blob from source either. So by now I didn't find any other way to test this opcode rather than manually code it in the effect blob.
Maybe I would rather generate the code for all the other cases too? It won't make this part less evident but it will be a uniform approach.
Well, yes, I mean creating by hand an effect blob with the specific instruction encodings you're currently testing in test_effect_preshader_ops(). You might make a separate preshader / state for each instruction or not (or anything in between), whatever is easier.
2017-05-11 17:32 GMT+02:00 Matteo Bruni <matteo.mystral(a)gmail.com>:
2017-05-11 17:08 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
The problem is I met this opcode in the wild and thus added it in the first version of preshader implementation (and can "model" and test it by manually editing preshader code), but could not make native d3dx compiler to generate it so far. Whatever I tried in HLSL which would be a straightforward use of 'cmp' resulted in different opcode sequences. It was either generated by some earlier versions of compiler or I just could not find the conditions under which it will be used. As for adding some other 3-parameter opcode to effect blob, while scanning all the opcodes in the 3 hex digit opcode range I have found just one other 3 parameter opcode 'movc', but it seem to always return 0 and I cannot generate it in effect blob from source either. So by now I didn't find any other way to test this opcode rather than manually code it in the effect blob.
Maybe I would rather generate the code for all the other cases too? It won't make this part less evident but it will be a uniform approach.
Well, yes, I mean creating by hand an effect blob with the specific instruction encodings you're currently testing in test_effect_preshader_ops(). You might make a separate preshader / state for each instruction or not (or anything in between), whatever is easier.
Not sure we're on the same page so let me try to say it differently: what I'd like to see is a hand-crafted effect blob which contains preshader(s) which use those "interesting" instructions. That would be used in a test replacing test_effect_preshader_ops(). Hand-crafting an effect file is probably going to be a bit tedious (e.g. the typedefs can get quite convoluted) so I suggest you to start by picking a few pieces of one of the existing effect blobs (e.g. test_effect_preshader_effect_blob[]) and fixing up the offsets and counts.
On 05/11/2017 06:42 PM, Matteo Bruni wrote:
2017-05-11 17:32 GMT+02:00 Matteo Bruni <matteo.mystral(a)gmail.com>:
2017-05-11 17:08 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
The problem is I met this opcode in the wild and thus added it in the first version of preshader implementation (and can "model" and test it by manually editing preshader code), but could not make native d3dx compiler to generate it so far. Whatever I tried in HLSL which would be a straightforward use of 'cmp' resulted in different opcode sequences. It was either generated by some earlier versions of compiler or I just could not find the conditions under which it will be used. As for adding some other 3-parameter opcode to effect blob, while scanning all the opcodes in the 3 hex digit opcode range I have found just one other 3 parameter opcode 'movc', but it seem to always return 0 and I cannot generate it in effect blob from source either. So by now I didn't find any other way to test this opcode rather than manually code it in the effect blob.
Maybe I would rather generate the code for all the other cases too? It won't make this part less evident but it will be a uniform approach. Well, yes, I mean creating by hand an effect blob with the specific instruction encodings you're currently testing in test_effect_preshader_ops(). You might make a separate preshader / state for each instruction or not (or anything in between), whatever is easier. Not sure we're on the same page so let me try to say it differently: what I'd like to see is a hand-crafted effect blob which contains preshader(s) which use those "interesting" instructions. That would be used in a test replacing test_effect_preshader_ops().
Hand-crafting an effect file is probably going to be a bit tedious (e.g. the typedefs can get quite convoluted) so I suggest you to start by picking a few pieces of one of the existing effect blobs (e.g. test_effect_preshader_effect_blob[]) and fixing up the offsets and counts.
The only feasible way for me to do and support it is to compile a small "sample" effect blob, take my current code as a "compiler" which will "fix" instructions and generate a blob and put a ready blob into the test. Do you think it really worth it? Instead of the table with parameters where one can see which values are tested, and some code which makes the localized changes to it, we will get a binary blob in test which has no public tool to compile it from any source or to decompile it. What if anyone else will want to add another test case for some instruction with different parameters? Adding such a test will require a full understanding of effect binary structure and will be a sort of nightmare.
2017-05-11 17:52 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 06:42 PM, Matteo Bruni wrote:
2017-05-11 17:32 GMT+02:00 Matteo Bruni <matteo.mystral(a)gmail.com>:
2017-05-11 17:08 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
The problem is I met this opcode in the wild and thus added it in the first version of preshader implementation (and can "model" and test it by manually editing preshader code), but could not make native d3dx compiler to generate it so far. Whatever I tried in HLSL which would be a straightforward use of 'cmp' resulted in different opcode sequences. It was either generated by some earlier versions of compiler or I just could not find the conditions under which it will be used. As for adding some other 3-parameter opcode to effect blob, while scanning all the opcodes in the 3 hex digit opcode range I have found just one other 3 parameter opcode 'movc', but it seem to always return 0 and I cannot generate it in effect blob from source either. So by now I didn't find any other way to test this opcode rather than manually code it in the effect blob.
Maybe I would rather generate the code for all the other cases too? It won't make this part less evident but it will be a uniform approach.
Well, yes, I mean creating by hand an effect blob with the specific instruction encodings you're currently testing in test_effect_preshader_ops(). You might make a separate preshader / state for each instruction or not (or anything in between), whatever is easier.
Not sure we're on the same page so let me try to say it differently: what I'd like to see is a hand-crafted effect blob which contains preshader(s) which use those "interesting" instructions. That would be used in a test replacing test_effect_preshader_ops().
Hand-crafting an effect file is probably going to be a bit tedious (e.g. the typedefs can get quite convoluted) so I suggest you to start by picking a few pieces of one of the existing effect blobs (e.g. test_effect_preshader_effect_blob[]) and fixing up the offsets and counts.
The only feasible way for me to do and support it is to compile a small "sample" effect blob, take my current code as a "compiler" which will "fix" instructions and generate a blob and put a ready blob into the test. Do you think it really worth it? Instead of the table with parameters where one can see which values are tested, and some code which makes the localized changes to it, we will get a binary blob in test which has no public tool to compile it from any source or to decompile it. What if anyone else will want to add another test case for some instruction with different parameters? Adding such a test will require a full understanding of effect binary structure and will be a sort of nightmare.
It is already the case though, you're changing the blob at runtime in the test and that implies knowing the encoding of the effect bytecode. The blob hacking as it is now is also pretty obscure and fragile (e.g. see the accidental overwriting of the CTAB you fixed recently). If anything, an effect blob annotated with comments should be MORE readable and allows you to test exactly what you want. About maintaining it / adding more tests, it shouldn't be too terrible. The most annoying change I can think of is probably adding new effect parameters. You could put a few extra (unused) parameters right away to reduce that risk. Same for extra states / preshaders, maybe. It might make sense to create a small tool to help with the effect blob construction. For example, something that fills the various offsets for you, like relocations. If so, it should come up quite naturally while preparing the blob. Maybe I'm wrong and this is utterly insane, but it doesn't feel like it to me at the moment (but then again that's what fools think all the time, I guess...) Try to give it a shot though. If it's terrible and ugly and kills kittens, just drop everything and continue using the current test scheme.
On 05/11/2017 07:15 PM, Matteo Bruni wrote:
2017-05-11 17:52 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 06:42 PM, Matteo Bruni wrote:
The only feasible way for me to do and support it is to compile a small "sample" effect blob, take my current code as a "compiler" which will "fix" instructions and generate a blob and put a ready blob into the test. Do you think it really worth it? Instead of the table with parameters where one can see which values are tested, and some code which makes the localized changes to it, we will get a binary blob in test which has no public tool to compile it from any source or to decompile it. What if anyone else will want to add another test case for some instruction with different parameters? Adding such a test will require a full understanding of effect binary structure and will be a sort of nightmare. It is already the case though, you're changing the blob at runtime in the test and that implies knowing the encoding of the effect bytecode. The blob hacking as it is now is also pretty obscure and fragile (e.g. see the accidental overwriting of the CTAB you fixed recently). Well yes, I am changing the blob, but just a very small part of it. The preshader encoding is not that terrible as for the whole effect, its format is pretty straightforward and does not have any cross references. Please note that if you want to add some test now for a new preshader opcode of already supported number of arguments, or extend the test for existing opcode with more cases, you don't have to change that part, just add the readable line to the test table with values. And you don't have to manually decompile the blob or trust the comments to see what values are tested there.
If anything, an effect blob annotated with comments should be MORE readable and allows you to test exactly what you want. About maintaining it / adding more tests, it shouldn't be too terrible. The most annoying change I can think of is probably adding new effect parameters. You could put a few extra (unused) parameters right away to reduce that risk. Same for extra states / preshaders, maybe.
It might make sense to create a small tool to help with the effect blob construction. For example, something that fills the various offsets for you, like relocations. If so, it should come up quite naturally while preparing the blob. Any change of any field in size will require updating offsets / sizes in the blob. I can't even think of maintaining that fully manually. At the same time, creating a stand alone small "compiler" is not terribly difficult (if it relies on the precompiled effect and not actually creating everything from scratch and updating every size / offset)? If to make a tool which will actually "fixup" the blob for offsets, it is a bit bigger thing though, doesn't it look a bit too much for a private tool used for nothing but supporting the tests of a few opcodes?
Maybe I'm wrong and this is utterly insane, but it doesn't feel like it to me at the moment (but then again that's what fools think all the time, I guess...) Try to give it a shot though. If it's terrible and ugly and kills kittens, just drop everything and continue using the current test scheme.
The initially added opcode were tested in the test blob (which can be (re)compiled by fxc and had the source in the test file). Then I suggested this new test function some time ago for easier adding the opcodes without changing the blob, which is there now. If we forget about the 'cmp' instruction for a moment it is not hard to just compile (separate) effect blob(s) for every instruction and place blobs in the test. It though looks like a step backward to me as I was sure the table based "blob-free" test for a bunch of instructions, extendable for new tests with the new data as easy as adding just one line with parameter values is better. After that, I could craft just once a blob with 'cmp' instruction, as it is the only instruction I encountered so far which I cannot generate by now (apart from a bunch of instructions which return 0 for any parameters, which I suppose are never used by compiler and probably should not be added or tested). But to be honest I would like to avoid that if possible, at least until some new unexpected opcodes / cases will be discovered which would make the things worse. Maybe alternative somewhat "intermediate" approach would be to build just a preshader blob for "cmp" instruction instead of generating it and test it in a separate test function? This would be a ready blob for preshader to be just copied to the specified location in the existing effect blob, while the existing code in test_preshader_opcode would be left unchanged.
2017-05-11 18:48 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 07:15 PM, Matteo Bruni wrote:
It might make sense to create a small tool to help with the effect blob construction. For example, something that fills the various offsets for you, like relocations. If so, it should come up quite naturally while preparing the blob.
Any change of any field in size will require updating offsets / sizes in the blob.
It depends on the change, e.g. adding a preshader instruction shouldn't require too many fixups elsewhere.
I can't even think of maintaining that fully manually. At the same time, creating a stand alone small "compiler" is not terribly difficult (if it relies on the precompiled effect and not actually creating everything from scratch and updating every size / offset)? If to make a tool which will actually "fixup" the blob for offsets, it is a bit bigger thing though, doesn't it look a bit too much for a private tool used for nothing but supporting the tests of a few opcodes?
I don't know what tools will be useful in practice. Generally that becomes pretty clear once one starts working on it. The relocation thing was just an idea and I was thinking of something pretty ad-hoc, like driven by a table filled by hand and / or with the "base" blob containing some special symbols to be replaced by offsets, or something like that. FWIW it doesn't necessarily have to be a 100% private ad-hoc tool used only for this test. We want to support generating effects from d3dcompiler at some point and for that we'll need to extend bytecodewriter.c to generate effects bytecode. One way to do that is to introduce a struct bwriter_effect storing a representation of all the parameters and techniques (and thus passes, states, ...) of an effect, similar to the current struct bwriter_shader, and a bwriter_write_effect() function which serializes that to bytecode, in the same vein as SlWriteBytecode(). The tool itself could then simply build a struct bwriter_effect with all the stuff you need for the test and call bwriter_write_effect() on it. I don't know if it makes sense to get there right now. It's certainly overkill for the test alone but on the other hand it would be an excuse to implement a d3dcompiler feature which will be eventually useful for the compiler itself.
Maybe I'm wrong and this is utterly insane, but it doesn't feel like it to me at the moment (but then again that's what fools think all the time, I guess...) Try to give it a shot though. If it's terrible and ugly and kills kittens, just drop everything and continue using the current test scheme.
The initially added opcode were tested in the test blob (which can be (re)compiled by fxc and had the source in the test file). Then I suggested this new test function some time ago for easier adding the opcodes without changing the blob, which is there now. If we forget about the 'cmp' instruction for a moment it is not hard to just compile (separate) effect blob(s) for every instruction and place blobs in the test. It though looks like a step backward to me as I was sure the table based "blob-free" test for a bunch of instructions, extendable for new tests with the new data as easy as adding just one line with parameter values is better. After that, I could craft just once a blob with 'cmp' instruction, as it is the only instruction I encountered so far which I cannot generate by now (apart from a bunch of instructions which return 0 for any parameters, which I suppose are never used by compiler and probably should not be added or tested). But to be honest I would like to avoid that if possible, at least until some new unexpected opcodes / cases will be discovered which would make the things worse.
Maybe alternative somewhat "intermediate" approach would be to build just a preshader blob for "cmp" instruction instead of generating it and test it in a separate test function? This would be a ready blob for preshader to be just copied to the specified location in the existing effect blob, while the existing code in test_preshader_opcode would be left unchanged.
That doesn't help much with the issues I mentioned with the current tests though.
On 05/11/2017 08:48 PM, Matteo Bruni wrote:
2017-05-11 18:48 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
FWIW it doesn't necessarily have to be a 100% private ad-hoc tool used only for this test. We want to support generating effects from d3dcompiler at some point and for that we'll need to extend bytecodewriter.c to generate effects bytecode. One way to do that is to introduce a struct bwriter_effect storing a representation of all the parameters and techniques (and thus passes, states, ...) of an effect, similar to the current struct bwriter_shader, and a bwriter_write_effect() function which serializes that to bytecode, in the same vein as SlWriteBytecode(). The tool itself could then simply build a struct bwriter_effect with all the stuff you need for the test and call bwriter_write_effect() on it. I don't know if it makes sense to get there right now. It's certainly overkill for the test alone but on the other hand it would be an excuse to implement a d3dcompiler feature which will be eventually useful for the compiler itself. Implementing an effect compiler is an interesting thing, though a big one. I didn't look at what is there in the current effect compiler framework yet though, getting into it alone requires some time.
Maybe I'm wrong and this is utterly insane, but it doesn't feel like it to me at the moment (but then again that's what fools think all the time, I guess...) Try to give it a shot though. If it's terrible and ugly and kills kittens, just drop everything and continue using the current test scheme.
Maybe alternative somewhat "intermediate" approach would be to build just a preshader blob for "cmp" instruction instead of generating it and test it in a separate test function? This would be a ready blob for preshader to be just copied to the specified location in the existing effect blob, while the existing code in test_preshader_opcode would be left unchanged. That doesn't help much with the issues I mentioned with the current tests though.
It is possible to generate corrupted effect blob, exactly the same way the code was corrupting it before that fix. Anyway, I understand your reasoning, but maybe there is some simpler option to add a test for 'cmp' instruction and maybe improve the existing test then stepping towards effect compiler?
2017-05-11 20:23 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 08:48 PM, Matteo Bruni wrote:
2017-05-11 18:48 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
FWIW it doesn't necessarily have to be a 100% private ad-hoc tool used only for this test. We want to support generating effects from d3dcompiler at some point and for that we'll need to extend bytecodewriter.c to generate effects bytecode. One way to do that is to introduce a struct bwriter_effect storing a representation of all the parameters and techniques (and thus passes, states, ...) of an effect, similar to the current struct bwriter_shader, and a bwriter_write_effect() function which serializes that to bytecode, in the same vein as SlWriteBytecode(). The tool itself could then simply build a struct bwriter_effect with all the stuff you need for the test and call bwriter_write_effect() on it. I don't know if it makes sense to get there right now. It's certainly overkill for the test alone but on the other hand it would be an excuse to implement a d3dcompiler feature which will be eventually useful for the compiler itself.
Implementing an effect compiler is an interesting thing, though a big one. I didn't look at what is there in the current effect compiler framework yet though, getting into it alone requires some time
You don't need the whole thing, just the "bytecode writer" part, which is pretty separate from the rest.
Maybe I'm wrong and this is utterly insane, but it doesn't feel like it to me at the moment (but then again that's what fools think all the time, I guess...) Try to give it a shot though. If it's terrible and ugly and kills kittens, just drop everything and continue using the current test scheme.
Maybe alternative somewhat "intermediate" approach would be to build just a preshader blob for "cmp" instruction instead of generating it and test it in a separate test function? This would be a ready blob for preshader to be just copied to the specified location in the existing effect blob, while the existing code in test_preshader_opcode would be left unchanged.
That doesn't help much with the issues I mentioned with the current tests though.
It is possible to generate corrupted effect blob, exactly the same way the code was corrupting it before that fix. Anyway, I understand your reasoning, but maybe there is some simpler option to add a test for 'cmp' instruction and maybe improve the existing test then stepping towards effect compiler?
As I said in my first reply, I'm okay with accepting the original patch. I'm not particularly happy with it and I hoped you would give at least a half-hearted try to the alternative but I'm not vetoing it.
On 05/11/2017 10:17 PM, Matteo Bruni wrote:
As I said in my first reply, I'm okay with accepting the original patch. I'm not particularly happy with it and I hoped you would give at least a half-hearted try to the alternative but I'm not vetoing it.
I will look at "bytecode writer" to see how long it might be to generate something. But to generate a single preshader at full I will probably need to generate "PRSI" section which we are not even parsing now, also there are "unknown DWORDS" skipped but it is subject to testing if native d3dx will go well without them filled right, and maybe some other issues which I can't guess now before looking into bytecode writer. Of course there is still an option to craft preshaders from a ready effect blob fixing opcodes, pretty much the same way they are generated now inside the test but putting a ready blob into the test file. If you are sure that it is a better approach I can do that, adding an array of blobs instead of array of ops + parameters.
2017-05-11 22:00 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 10:17 PM, Matteo Bruni wrote:
As I said in my first reply, I'm okay with accepting the original patch. I'm not particularly happy with it and I hoped you would give at least a half-hearted try to the alternative but I'm not vetoing it.
I will look at "bytecode writer" to see how long it might be to generate something. But to generate a single preshader at full I will probably need to generate "PRSI" section which we are not even parsing now, also there are "unknown DWORDS" skipped but it is subject to testing if native d3dx will go well without them filled right
That's an opportunity for more tests, which is good :)
Of course there is still an option to craft preshaders from a ready effect blob fixing opcodes, pretty much the same way they are generated now inside the test but putting a ready blob into the test file. If you are sure that it is a better approach I can do that, adding an array of blobs instead of array of ops + parameters.
What do you mean exactly? I'd be okay with creating the effect blob by writing a "skeleton" effect, compiling it with fxc and then manually adding / replacing some preshader instructions in the bytecode with those you want to test, fixing up the affected offsets / sizes / counts to account for the changed stuff. With a suitable skeleton the changes should be pretty localized and you could document them in comments inside the blob array (and still document the original effect's source). In the end it would be pretty similar to the current test except that 1. you do the changes offline and not at runtime 2. the changes have comments right in the bytecode 3. you don't need to hack a huge effect with a lot of unrelated stuff. It could also be multiple effects for different instruction tests but it seems simpler to put everything into one.
On 05/11/2017 11:57 PM, Matteo Bruni wrote:
> 2017-05-11 22:00 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
>> On 05/11/2017 10:17 PM, Matteo Bruni wrote:
>>>
>>> As I said in my first reply, I'm okay with accepting the original
>>> patch. I'm not particularly happy with it and I hoped you would give
>>> at least a half-hearted try to the alternative but I'm not vetoing it.
>>>
>> I will look at "bytecode writer" to see how long it might be to generate
>> something. But to generate a single preshader at full I will probably need
>> to generate "PRSI" section which we are not even parsing now, also there are
>> "unknown DWORDS" skipped but it is subject to testing if native d3dx will go
>> well without them filled right
> That's an opportunity for more tests, which is good :)
Yes, but such tests are not much related to the functionality I was keen
to bring to some more or less finalized state. Guessing these
(partially) unknown fields and structures are of interest for effect
compiler implementation only in my understanding, but if was starting
that I would think the primary thing to do is HLSL shader compiler, as
it is the fully independent part of effect compilation (usable alone),
without which effect compiler would be mostly unusable anywhere but some
specially crafted tests without shaders.
>> Of course there is still an
>> option to craft preshaders from a ready effect blob fixing opcodes, pretty
>> much the same way they are generated now inside the test but putting a ready
>> blob into the test file. If you are sure that it is a better approach I can
>> do that, adding an array of blobs instead of array of ops + parameters.
> What do you mean exactly? I'd be okay with creating the effect blob by
> writing a "skeleton" effect, compiling it with fxc and then manually
> adding / replacing some preshader instructions in the bytecode with
> those you want to test, fixing up the affected offsets / sizes /
> counts to account for the changed stuff.
Yes, it is pretty much what I mean here, but I don't really need to
update any sizes as the actual preshader can take less space then it has
in effect blob (that's what I do now in 3 argument function test
generation where I am using a space from longer preshader). Preshader
has instruction count in the beginning and neither native d3dx nor we
care if there are any extra bytes after preshader end (it also has a
sort of end marker for which no one cares either). So the essence of
such a change is bringing effect change offline.
> With a suitable skeleton the changes should be pretty localized and
> you could document them in comments inside the blob array (and still
> document the original effect's source). In the end it would be pretty
> similar to the current test except that 1. you do the changes offline
> and not at runtime 2. the changes have comments right in the bytecode
> 3. you don't need to hack a huge effect with a lot of unrelated stuff.
I would suggest doing the following:
1. prepare a separate blob for the instructions testing not to mess up
with a copy of a big one which suits for different tests;
2. comment the fields directly related to the preshader being changed
right in the effect blob;
3. leave the code modifying the blob online but add comments for
setting fields.
Otherwise I can prepare the single blob with everything buildin,
mark preshader code in comments pointing where manual changes are, and
leave test function just to set parameters. I could possibly do the same
in the very beginning when was adding this test, somehow this was not
discussed that time. But actually I was previously sure that having all
that binary blobs in tests is an ugly but unavoidable measure, and
generating what we need to test based on some patterns or readable data
is preferred if possible, as makes those tests easier verifiable and
easier producible.
2017-05-11 23:57 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 11:57 PM, Matteo Bruni wrote:
2017-05-11 22:00 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/11/2017 10:17 PM, Matteo Bruni wrote:
As I said in my first reply, I'm okay with accepting the original patch. I'm not particularly happy with it and I hoped you would give at least a half-hearted try to the alternative but I'm not vetoing it.
I will look at "bytecode writer" to see how long it might be to generate something. But to generate a single preshader at full I will probably need to generate "PRSI" section which we are not even parsing now, also there are "unknown DWORDS" skipped but it is subject to testing if native d3dx will go well without them filled right
That's an opportunity for more tests, which is good :)
Yes, but such tests are not much related to the functionality I was keen to bring to some more or less finalized state. Guessing these (partially) unknown fields and structures are of interest for effect compiler implementation only in my understanding
Well, if it turns out that they have some effect on native d3dx9 it means we probably should also do something in our d3dx9 with that data. It's unlikely to be the case but in principle that's a valid test.
but if was starting that I would think the primary thing to do is HLSL shader compiler, as it is the fully independent part of effect compilation (usable alone), without which effect compiler would be mostly unusable anywhere but some specially crafted tests without shaders.
Yeah, that would be the logical order of things.
Of course there is still an option to craft preshaders from a ready effect blob fixing opcodes, pretty much the same way they are generated now inside the test but putting a ready blob into the test file. If you are sure that it is a better approach I can do that, adding an array of blobs instead of array of ops + parameters.
What do you mean exactly? I'd be okay with creating the effect blob by writing a "skeleton" effect, compiling it with fxc and then manually adding / replacing some preshader instructions in the bytecode with those you want to test, fixing up the affected offsets / sizes / counts to account for the changed stuff.
Yes, it is pretty much what I mean here, but I don't really need to update any sizes as the actual preshader can take less space then it has in effect blob (that's what I do now in 3 argument function test generation where I am using a space from longer preshader). Preshader has instruction count in the beginning and neither native d3dx nor we care if there are any extra bytes after preshader end (it also has a sort of end marker for which no one cares either). So the essence of such a change is bringing effect change offline.
Sure.
With a suitable skeleton the changes should be pretty localized and you could document them in comments inside the blob array (and still document the original effect's source). In the end it would be pretty similar to the current test except that 1. you do the changes offline and not at runtime 2. the changes have comments right in the bytecode 3. you don't need to hack a huge effect with a lot of unrelated stuff.
I would suggest doing the following: 1. prepare a separate blob for the instructions testing not to mess up with a copy of a big one which suits for different tests; 2. comment the fields directly related to the preshader being changed right in the effect blob; 3. leave the code modifying the blob online but add comments for setting fields.
Otherwise I can prepare the single blob with everything buildin, mark preshader code in comments pointing where manual changes are, and leave test function just to set parameters. I could possibly do the same in the very beginning when was adding this test, somehow this was not discussed that time. But actually I was previously sure that having all that binary blobs in tests is an ugly but unavoidable measure, and generating what we need to test based on some patterns or readable data is preferred if possible, as makes those tests easier verifiable and easier producible.
This "otherwise" sounds like what I last proposed. Yeah, ideally we'd always generate the relevant preshaders instructions from source but as you noticed that isn't always doable. That means resorting to some kind of hack and, as far as hacks go, manually modifying the bytecode offline while adding comments looks like the least ugly option to me at the moment.
On 05/12/2017 01:16 AM, Matteo Bruni wrote:
2017-05-11 23:57 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
I would suggest doing the following: 1. prepare a separate blob for the instructions testing not to mess up with a copy of a big one which suits for different tests; 2. comment the fields directly related to the preshader being changed right in the effect blob; 3. leave the code modifying the blob online but add comments for setting fields.
Otherwise I can prepare the single blob with everything buildin, mark preshader code in comments pointing where manual changes are, and leave test function just to set parameters. I could possibly do the same in the very beginning when was adding this test, somehow this was not discussed that time. But actually I was previously sure that having all that binary blobs in tests is an ugly but unavoidable measure, and generating what we need to test based on some patterns or readable data is preferred if possible, as makes those tests easier verifiable and easier producible. This "otherwise" sounds like what I last proposed. Yeah, ideally we'd always generate the relevant preshaders instructions from source but as you noticed that isn't always doable. That means resorting to some kind of hack and, as far as hacks go, manually modifying the bytecode offline while adding comments looks like the least ugly option to me at the moment. Should I make a single blob or separate for each instruction? Single blob will require an update on each instruction added, while separate (rather small) blobs for each will be the same apart from a few bytes related to the operation being tested. Separate blobs look slightly preferable to me, as don't require using and enumerating numerous unrelated states in test function, and it is easier to find something in the blob which has just one state and one preshader. If to go with separate blobs, could you please advise how to best send it taking into account existing test? As a single patch replacing the test with all the blobs at once, or a few ones leaving existing test and removing it in the last one?
On 05/11/2017 06:32 PM, Matteo Bruni wrote:
2017-05-11 17:08 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: Well, yes, I mean creating by hand an effect blob with the specific instruction encodings you're currently testing in test_effect_preshader_ops(). You might make a separate preshader / state for each instruction or not (or anything in between), whatever is easier.
So do you suggest to make the complete effect blob which is not generated by compiler, with all the tested operations built in there? If this is the case I am afraid it will make any change / adding instruction test or input parameters combination a nightmare, unless I am going to make a compiler for that, and much less readable than it is now with the table of tested instructions. Or do you mean just different (small) compiled effect blob with "placeholder" instructions to set the opcodes, pretty much as it is done now, just in a dedicated smaller blob? Then the code will be pretty much the same to what it is now though, just different effect blob. Or am I misunderstanding you entirely?
participants (2)
-
Matteo Bruni -
Paul Gofman