1689316 - Simplify if statements generated from switch statements in webrender shaders

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, task)

Description

[Jamie Nicol [:jnicol]]

In bug 1685563 I attempted to add an additional case to a switch statement in a shader. This caused some rendering issues on an Adreno 530 device (bug 1687554). My theory is that we hit a limit on the number of branches that the GPU can execute and it does strange things.

The problem is exacerbated by the fact that glslopt produces horrible output for switch statements: each case is turned in to an if of course, but there is also an additional if to check if we have breaked.

I think we've encountered similar issues in the past on other platforms: bug 1663344, and possibly here. I also recall component transfer tests failing on Pixel 2 when I initially started working on shader optimization, but by the time I was ready to land they magically worked.

If we manually replace all our switch statements with if-else statements then it fixes the bug. In fact doing just the text shader is enough for this bug, but since we've encountered similar problems before I think we should replace them all.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Bug 1689316 - Convert all switch statements to if-elses in webrender shaders. r?jrmuizel

We keep encountering issues on various platforms due to the usage of
switch statements, especially the optimized output produced by
glslopt. Replace all instances with if-else statements instead.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec84f8196c4c
Convert all switch statements to if-elses in webrender shaders. r=jrmuizel

Comment 3

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ec84f8196c4c

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1689510.
https://hg.mozilla.org/mozilla-central/rev/62eead3951fa7e3d40b2f96f814d5e6e80409342

Comment 5

[Dave Townsend [:mossop]]

Oh, I just disabled webrender with gfx.webrender.force-disabled and it went away.

Comment 6

[Markus Stange [:mstange]]

The black text happens with the Intel driver but not with the AMD driver.

+    if (color_mode == COLOR_MODE_ALPHA || color_mode == COLOR_MODE_BITMAP) {

This is an equality comparison with an integer. These have caused trouble in the past.

I am reminded of bug 1679700, where we also ran into this bug after changing a switch to an if (and also because the switch was causing problems on other drivers).

Comment 7

[Lee Salzman [:lsalzman]]

I am somewhat worried this would regress performance on SW-WR, in that I don't know that clang will be smart enough to identify and generate jump-tables or binary search anymore for the branch sequences. I would fear that it would only give us what we ask for in this case, which is just a series of brute force equality comparisons and branches. Doing this in a fragment shader would be especially not desirable...

Comment 8

[Jamie Nicol [:jnicol]]

Bug 1690027 should fix the Mac issue. And in order not to regress SWGL performance, the plan is to leave the switch statements alone in the unoptimized shader source code. I have found an easy way to make the optimizer output slightly simpler if statements for switches: The code is basically the same, except instead of if (break_var) condition = false we output condition = condition && !break_var. This halves the number of ifs, which is what was causing the issues.

We can in the future make the optimizer output even simpler if-else statements in the simple case (no fallthrough, etc). But for now, this fixes the issue at hand and allows us to progress with bug 1685563

Comment 9

[Jamie Nicol [:jnicol]]

Attachment: Bug 1689316 - Update glslopt to optimize shader switch statements in to fewer ifs. r?jrmuizel

We have encountered issues on some platforms due to a large number of
if statements in shaders. The shader optimizer previously generated
code with a large number of if statements, due to the way in which it
optimized switch statements.

Previously the optimizer output 2 if statements for every case in a
switch. First it ORs the "fallthrough" var with the case's
condition. Then sets the fallthrough var to false if the "break" var
is true. Then conditionally executes the case's instructions if
fallthrough is true. For example:

switch (uMode) {
case 0:
gl_Position = vec4(0.0);
break;
case 1:
gl_Position = vec4(1.0);
break;
}

becomes:

bool break_var = bool(0);
bool fallthrough_var = (0 == uMode);
if (break_var) fallthrough_var = bool(0);
if (fallthrough_var) {
gl_Position = vec4(0.0, 0.0, 0.0, 0.0);
break_var = bool(1);
};
fallthrough_var = (fallthrough_var || (1 == uMode));
if (break_var) fallthrough_var = bool(0);
if (fallthrough_var) {
gl_Position = vec4(1.0, 1.0, 1.0, 1.0);
break_var = bool(1);
};

This update removes one of these ifs, by ANDing the fallthrough_var
with !break_var rather than conditionally setting it to false. eg:

bool break_var = bool(0);
bool fallthrough_var = (0 == uMode);
if (fallthrough_var) {
gl_Position = vec4(0.0, 0.0, 0.0, 0.0);
break_var = bool(1);
};
fallthrough_var = (fallthrough_var || (1 == uMode));
fallthrough_var = (fallthrough_var && !(break_var));
if (fallthrough_var) {
gl_Position = vec4(1.0, 1.0, 1.0, 1.0);
break_var = bool(1);
};

This is logically equivalent but uses half as many if statements,
which helps to avoid driver bugs on some platforms.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d660edea73c1
Update glslopt to optimize shader switch statements in to fewer ifs. r=jrmuizel

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d660edea73c1