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

[Matteo Bruni]

2017-05-11 20:23 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> On 05/11/2017 08:48 PM, Matteo Bruni wrote:
>>
>> 2017-05-11 18:48 GMT+02:00 Paul Gofman <gofmanp at 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.