Disable optimizations for shaders with conditional discard in D3D9, and only use expanded

Disable optimizations for shaders with conditional discard in D3D9, and only use expanded
short-circuiting conditionals for expressions with potential side-effects.

Conservatively assume aggreate and selection operators have side effects for now.

BUG=
ANGLEBUG=486
R=geofflang@chromium.org, kbr@chromium.org, nicolas@transgaming.com, shannonwoods@chromium.org

Committed: https://code.google.com/p/angleproject/source/detail?r=d1ffb56

[Jamie Madill, 2013-10-16 19:17:14]

Please take a look!

[Geoff Lang, 2013-10-16 20:22:06]

LGTM

[Shannon Woods, 2013-10-17 18:55:42]

On 2013/10/16 19:17:14, Jamie Madill wrote:
> Please take a look!

Is this behavior something we might want to tuck behind a compilation or
run-time flag, so that it can be toggled if different D3D compilers don't
trigger the bug? 

Ideally, this should get reviewed by Nicolas as well.

[Jamie Madill, 2013-10-18 18:32:00]

On 2013/10/17 18:55:42, Shannon Woods wrote:
> On 2013/10/16 19:17:14, Jamie Madill wrote:
> > Please take a look!
> 
> Is this behavior something we might want to tuck behind a compilation or
> run-time flag, so that it can be toggled if different D3D compilers don't
> trigger the bug? 
> 
> Ideally, this should get reviewed by Nicolas as well.

Added nicolas to reviewers, and kbr to cc.

[kbr1, 2013-10-18 18:47:40]

Could you re-upload? The side-by-side diffs are broken.

[Jamie Madill, 2013-10-18 19:09:04]

On 2013/10/18 18:47:40, kbr1 wrote:
> Could you re-upload? The side-by-side diffs are broken.

Re-uploaded!

[kbr1, 2013-10-18 19:13:16]

LGTM with one minor issue.

https://codereview.appspot.com/14441075/diff/167001/src/libGLESv2/renderer/Re...
File src/libGLESv2/renderer/Renderer.h (right):

https://codereview.appspot.com/14441075/diff/167001/src/libGLESv2/renderer/Re...
src/libGLESv2/renderer/Renderer.h:210: virtual ShaderExecutable
*compileToExecutable(gl::InfoLog &infoLog, const char *shaderHLSL,
rx::ShaderType type, bool forceDisableOptimizations) = 0;
It would be better to define an enum for this rather than using a bool. The call
sites will be clearer.

[nicolas, 2013-10-25 06:19:27]

Perhaps this should be split into two patch sets since two things are done here.
One is to avoid rewriting short-circuiting operators with conditionals when
there are no side-effects, and the other is to disable D3D9 shader optimizations
when there are conditional discard operations.

https://codereview.appspot.com/14441075/diff/187001/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (right):

https://codereview.appspot.com/14441075/diff/187001/src/compiler/OutputHLSL.c...
src/compiler/OutputHLSL.cpp:201: if (mUsesConditionalRewriting && mUsesDiscard)
The rewriting of a short-circuiting expression using conditionals, and the
presence of a discard, does not mean the discard itself is conditional.

https://codereview.appspot.com/14441075/diff/187001/src/compiler/OutputHLSL.c...
src/compiler/OutputHLSL.cpp:1966: out << "s" <<
mUnfoldShortCircuit->getNextTemporaryIndex();
Should this also set mUsesConditionalRewriting? The name of that variable is
confusing to me, especially if it's intended just for detecting conditional
discards.

https://codereview.appspot.com/14441075/diff/187001/src/compiler/UnfoldShortC...
File src/compiler/UnfoldShortCircuit.cpp (right):

https://codereview.appspot.com/14441075/diff/187001/src/compiler/UnfoldShortC...
src/compiler/UnfoldShortCircuit.cpp:34: if (!node->getRight()->modifiesState())
What if the left node has side-effects, such as in "(x = y) = z;"?

https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate.h
File src/compiler/intermediate.h (right):

https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate...
src/compiler/intermediate.h:255: virtual bool modifiesState() const = 0;
I suggest changing this function's name to hasNoSideEffects. Having
'side-effects' is use more commonly than modifying 'state'. Also, with that name
it's clearer that when it returns false it's still possible that it actually has
no side-effects. Makes it easier to be conservative about it without causing
confusion.

https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate...
src/compiler/intermediate.h:409: virtual bool modifiesState() const;
Where's the implementation for this?

https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate...
src/compiler/intermediate.h:496: // Over-generally assume we mutate the state on
function calls or other aggregate operators
"Conservatively assume function calls and other aggregate operators have
side-effects"

[Jamie Madill, 2013-10-25 15:12:33]

Thanks for the review! I uploaded a new iteration.

On 2013/10/25 06:19:27, nicolas wrote:
> Perhaps this should be split into two patch sets

You mean two reviews, or multiple patches? The term patch set in this review
tool simply means the iteration of the upload; on my side I have multiple
patches. I'd recommend we hammer this one out using this review tool for now, to
make my life easier.

> 
>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/OutputHLSL.cpp
> File src/compiler/OutputHLSL.cpp (right):
> 
>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/OutputHLSL.c...
> src/compiler/OutputHLSL.cpp:201: if (mUsesConditionalRewriting &&
mUsesDiscard)
> The rewriting of a short-circuiting expression using conditionals, and the
> presence of a discard, does not mean the discard itself is conditional.

I changed my logic to less generally make assumptions -- only flag when there is
a discard in an if/ifelse block with a rewritten conditional.

>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/OutputHLSL.c...
> src/compiler/OutputHLSL.cpp:1966: out << "s" <<
> mUnfoldShortCircuit->getNextTemporaryIndex();
> Should this also set mUsesConditionalRewriting? The name of that variable is
> confusing to me, especially if it's intended just for detecting conditional
> discards.

Renamed to mUsesDiscardWriting. Could also go with
mUsesConditionalDiscardWriting but I like the brevity of the former.

>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/UnfoldShortC...
> File src/compiler/UnfoldShortCircuit.cpp (right):
> 
>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/UnfoldShortC...
> src/compiler/UnfoldShortCircuit.cpp:34: if
(!node->getRight()->modifiesState())
> What if the left node has side-effects, such as in "(x = y) = z;"?

This is fine, there's no condition here, so no unfolding. Expressions such as (a
&& (b = c) || d) are also fine, the nesting/traversing isn't interrupted by
"return true;".

>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate.h
> File src/compiler/intermediate.h (right):
> 
>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate...
> src/compiler/intermediate.h:255: virtual bool modifiesState() const = 0;
> I suggest changing this function's name to hasNoSideEffects. Having
> 'side-effects' is use more commonly than modifying 'state'.

Agreed, modifiesState was a relic of the original translator source commit
(revision 2!). Renamed to "hasSideEffects".

>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate...
> src/compiler/intermediate.h:409: virtual bool modifiesState() const;
> Where's the implementation for this?

Was already present -- in Intermediate.cpp. I did not modify it except to add
virtual (and now rename it).

>
https://codereview.appspot.com/14441075/diff/187001/src/compiler/intermediate...
> src/compiler/intermediate.h:496: // Over-generally assume we mutate the state
on
> function calls or other aggregate operators
> "Conservatively assume function calls and other aggregate operators have
> side-effects"

Agreed, thanks!

[Jamie Madill, 2013-10-25 15:13:45]

On 2013/10/25 15:12:33, Jamie Madill wrote:
> Renamed to mUsesDiscardWriting. Could also go with
> mUsesConditionalDiscardWriting but I like the brevity of the former.

*Rewriting

[Jamie Madill, 2013-11-04 16:09:36]

Committed patchset #6 manually as rd1ffb56 (presubmit successful).