Fix combiner independence #720

abaire · 2022-02-05T03:29:00Z

Makes combiner stage output independent when assigning multiple results. (e.g., R1 can be set with the previous value of R0 in the same stage as R0 is assigned with the previous value of R1 without the one affecting the other).

Test
Results

Fixes #51
(Apparently, based on reports in xemu#general) fixes #53

MasonT8198 · 2022-02-05T08:30:46Z

hw/xbox/nv2a/psh.c

    } else {
        mstring_unref(ab_dest);
        mstring_ref(ab_mapping);
        ab_dest = ab_mapping;
    }

    if (mstring_get_length(cd_dest)) {
-        mstring_append_fmt(ps->code, "%s.%s = clamp(%s(%s), -1.0, 1.0);\n",
-                           mstring_get_str(cd_dest), write_mask, caster, mstring_get_str(cd_mapping));


Why was this changed to cd.%s

Because he introduced two intermediate storage variables, specifically to fix this issue with

Because he introduced two intermediate storage variables, specifically to fix this issue with

Yeah, the fix is indeed to use intermediate vars so an output that sets a variable dependent on another variable in the output is not then dependent on the ordering they were specified as outputs (matching HW behavior).

E.g., if the output of the last stage had R0 set to (1.0, 0.0, 1.0) and the output of this stage is defined as R1 = R0 * T0 and R0 = T1 * C0 the final output should be: R1 = (1.0, 0.0, 1.0) * T0, R0 = T1 * C0 whereas the current xemu code makes the value of R1 depend on whether it was calculated in AB (it'll be correct) or CD (it'll be wrong, because R0 gets updated before R1 is calculated and it erroneously uses the new value).

PatrickvL · 2022-02-05T09:20:58Z

hw/xbox/nv2a/psh.c

    }
    if (!is_alpha && output.flags & PS_COMBINEROUTPUT_CD_BLUE_TO_ALPHA) {
-        mstring_append_fmt(ps->code, "%s.a = %s.b;\n",
-                           mstring_get_str(cd_dest), mstring_get_str(cd_dest));
+        mstring_append_fmt(ps->code, "cd.a = cd.b;\n");
    }

    MString *sum;
    if (output.muxsum_op == PS_COMBINEROUTPUT_AB_CD_SUM) {
        sum = mstring_from_fmt("(%s + %s)", mstring_get_str(ab), mstring_get_str(cd));


Shouldn't all 3 mux/sum paths read from the new ab and cd variables now?

'ab' and 'cd' in the C code actually contain the raw calculations (see ~line 419). They are only conditionally assigned to the glsl variables based on whether the results are referenced as part of the combiner output, so it would be unsafe to blindly use them here without adding code to blindly do the calculation and assignment to intermediates above.

Still, this currently results in duplicate expressions, which puts additional burden on the shader compiler, which could be avoided and reduced into a single assignment, when the ab and cd variables are used in these mux/sum paths. Indeed, that does require assignment to ab and/or cd, but that could also be evaluated upfront similar to the new assign_* booleans.
Such "require_*" booleans could also help fixing the potential "read without assignment" issue mentioned above.

Definitely agree with you, I think Matt's preference in general has been to put in the basic fix, then optimize, so I'm inclined to do cleanup in a second pass since I don't think this PR changes the duplication in the case where the AB/CD results are assigned along with the muxsum.

PatrickvL

Xemu isn't big on describing why code is written like it is, but this change is a good example of something that a future reader might see and think of "let's optimise this into a single assignment statement", overlooking the fact this specific construct is here to avoid sequential variable access (while hardware turns out to calculate in parallel). So IMHO even a single comment line on that would help tremendously here

PatrickvL · 2022-02-05T09:32:41Z

The pull request description should also mention that it includes handling of the PS_COMBINERCOUNT_MUX_MSB flag

abaire · 2022-02-05T14:55:09Z

The pull request description should also mention that it includes handling of the PS_COMBINERCOUNT_MUX_MSB flag

It doesn't intentionally. This PR is just dependent on several other of my PRs that need to be merged first. As far as I'm aware the xemu convention is one-fix-one-commit so any PR with multiple commits is generally squashed pre-review.

abaire · 2022-02-05T14:55:56Z

Xemu isn't big on describing why code is written like it is, but this change is a good example of something that a future reader might see and think of "let's optimise this into a single assignment statement", overlooking the fact this specific construct is here to avoid sequential variable access (while hardware turns out to calculate in parallel). So IMHO even a single comment line on that would help tremendously here

I don't disagree, but @mborgerson has asked me to remove comments a number of times so I'm defaulting to letting him ask for explanation where necessary.

PatrickvL · 2022-02-05T17:40:21Z

hw/xbox/nv2a/psh.c

    } else {
        mstring_unref(cd_dest);
        mstring_ref(cd_mapping);
        cd_dest = cd_mapping;
    }

    if (!is_alpha && output.flags & PS_COMBINEROUTPUT_AB_BLUE_TO_ALPHA) {
-        mstring_append_fmt(ps->code, "%s.a = %s.b;\n",
-                           mstring_get_str(ab_dest), mstring_get_str(ab_dest));
+        mstring_append(ps->code, "ab.a = ab.b;\n");


Admittedly an already existing issue might be that this flag is set, and thus ab is read, while ab isn't assigned to.

The same remark for cd below

In practice I'm not sure this matters; if the flag is set but the combiner is discarding the calculated AB value, it doesn't matter if this produces incorrect data. That said gating behind whether the values are to be used or not makes complete sense and is straightforward. This also caused me to add an additional test for the b2a behavior that pointed out the fact that I was introducing a bug where the alpha was never actually used in practice (fixed now) - thanks!

PatrickvL · 2022-02-05T17:53:19Z

hw/xbox/nv2a/psh.c

    }
    if (!is_alpha && output.flags & PS_COMBINEROUTPUT_CD_BLUE_TO_ALPHA) {
-        mstring_append_fmt(ps->code, "%s.a = %s.b;\n",
-                           mstring_get_str(cd_dest), mstring_get_str(cd_dest));
+        mstring_append_fmt(ps->code, "cd.a = cd.b;\n");
    }

    MString *sum;
    if (output.muxsum_op == PS_COMBINEROUTPUT_AB_CD_SUM) {
        sum = mstring_from_fmt("(%s + %s)", mstring_get_str(ab), mstring_get_str(cd));


Still, this currently results in duplicate expressions, which puts additional burden on the shader compiler, which could be avoided and reduced into a single assignment, when the ab and cd variables are used in these mux/sum paths. Indeed, that does require assignment to ab and/or cd, but that could also be evaluated upfront similar to the new assign_* booleans.
Such "require_*" booleans could also help fixing the potential "read without assignment" issue mentioned above.

ko81e24wy · 2022-02-07T17:49:25Z

this PR fix lighting problem of NBA LIVE 2005
master

PR

mborgerson · 2022-04-24T22:11:44Z

Awesome, thank you!

MasonT8198 reviewed Feb 5, 2022

View reviewed changes

PatrickvL reviewed Feb 5, 2022

View reviewed changes

abaire force-pushed the fix_combiner_independence branch from e8c006b to d22593e Compare February 5, 2022 18:52

abaire mentioned this pull request Feb 6, 2022

Title Name: Elder Scrolls III, The Morrowind - Game of the Year Edition (USA) #469

Closed

PatrickvL mentioned this pull request Feb 6, 2022

Grabbed by the Ghoulies: Missing geometry #721

Closed

RageXbox mentioned this pull request Mar 12, 2022

Handling of final combiner flags is silently unimplemented #765

Closed

abaire force-pushed the fix_combiner_independence branch from d22593e to 52122bc Compare March 14, 2022 22:32

abaire mentioned this pull request Mar 19, 2022

Generated frag shader may assign alpha channel of dot product statement #54

Closed

abaire force-pushed the fix_combiner_independence branch 2 times, most recently from 800fec1 to f719947 Compare March 19, 2022 15:29

nv2a: Make combiner assignment independent

c0a378a

mborgerson force-pushed the fix_combiner_independence branch from f719947 to c0a378a Compare March 23, 2022 00:42

RageXbox mentioned this pull request Apr 13, 2022

nv2a: Implement shadow samplers #840

Merged

abaire mentioned this pull request Apr 13, 2022

Some games lighting effects dont render correctly. #586

Open

mborgerson merged commit 4c83427 into xemu-project:master Apr 24, 2022

abaire deleted the fix_combiner_independence branch April 24, 2022 22:22

abaire mentioned this pull request May 30, 2022

Need for Speed Most Wanted: Missing orange/yellow color filter #890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix combiner independence #720

Fix combiner independence #720

abaire commented Feb 5, 2022 •

edited

MasonT8198 Feb 5, 2022

PatrickvL Feb 5, 2022

abaire Feb 5, 2022

PatrickvL Feb 5, 2022 •

edited

abaire Feb 5, 2022

PatrickvL Feb 5, 2022

abaire Feb 5, 2022

PatrickvL left a comment

PatrickvL commented Feb 5, 2022

abaire commented Feb 5, 2022

abaire commented Feb 5, 2022

PatrickvL Feb 5, 2022

abaire Feb 5, 2022

PatrickvL Feb 5, 2022

ko81e24wy commented Feb 7, 2022

mborgerson commented Apr 24, 2022

Fix combiner independence #720

Fix combiner independence #720

Conversation

abaire commented Feb 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatrickvL Feb 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatrickvL left a comment

Choose a reason for hiding this comment

PatrickvL commented Feb 5, 2022

abaire commented Feb 5, 2022

abaire commented Feb 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ko81e24wy commented Feb 7, 2022

mborgerson commented Apr 24, 2022

abaire commented Feb 5, 2022 •

edited

PatrickvL Feb 5, 2022 •

edited