[PATCH 8/9] d3dx9: Add test for 'cmp' preshader opcode.

[Paul Gofman]

On 05/11/2017 07:15 PM, Matteo Bruni wrote:
> 2017-05-11 17:52 GMT+02:00 Paul Gofman <gofmanp at 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.