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

[Matteo Bruni]

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