Add an option in ANGLE shader translator to initialize gl_Position to vec4(0.0, 0.0, 0.0, 1.0).

Add an option in ANGLE shader translator to initialize gl_Position to vec4(0.0, 0.0, 0.0, 1.0).

This is to work around driver bugs where shader compile or program link would fail incorrectly if gl_Position is not set in vertex shader.

At the moment at least Linux NVIDIA driver has this bug.

ANGLEBUG=472
R=alokp@chromium.org, kbr@chromium.org

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

[Zhenyao Mo, 2013-09-03 23:30:21]

Please review.

Will setup a driver-bug-workaround on the chromium side for this, and turn it on
for Linux NVIDIA only.

Tested with webgl conformance tests and a few webgl demos.

[kbr1, 2013-09-04 00:23:25]

The overall idea is good but I'd like to see a cleanup made.

https://codereview.appspot.com/13509043/diff/6001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/13509043/diff/6001/include/GLSLANG/ShaderLang....
include/GLSLANG/ShaderLang.h:188: // vertex shader, and has no effects in
fragment shader.
Could you clarify this sentence a little? (Fixes for minor typos included.)
Something like:

"This flag initializes gl_Position to vec4(0.0, 0.0, 0.0, 1.0) at the beginning
of the vertex shader, and has no effect in the fragment shader."

Could you please also add another sentence like: "This flag is intended as a
workaround for drivers which incorrectly fail to link programs if gl_Position is
not written."

https://codereview.appspot.com/13509043/diff/6001/src/compiler/ShHandle.h
File src/compiler/ShHandle.h (right):

https://codereview.appspot.com/13509043/diff/6001/src/compiler/ShHandle.h#new...
src/compiler/ShHandle.h:99: virtual void translate(TIntermNode* root, bool
initGLPosition) = 0;
Adding this boolean argument to the translate() method is ugly. Unless it's
impossible, please add a query similar to enforcePackingRestrictions(), below.

https://codereview.appspot.com/13509043/diff/6001/src/compiler/TranslatorESSL...
File src/compiler/TranslatorESSL.cpp (right):

https://codereview.appspot.com/13509043/diff/6001/src/compiler/TranslatorESSL...
src/compiler/TranslatorESSL.cpp:31: outputESSL.enableInitGLPosition();
Instead of calling a method on the outputESSL object, please pass the flag as an
argument to the constructor, similarly to how the index clamping strategy and
hash function are handled.

[Zhenyao Mo, 2013-09-04 01:29:44]

Please take another look.

https://codereview.appspot.com/13509043/diff/6001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/13509043/diff/6001/include/GLSLANG/ShaderLang....
include/GLSLANG/ShaderLang.h:188: // vertex shader, and has no effects in
fragment shader.
On 2013/09/04 00:23:25, kbr1 wrote:
> Could you clarify this sentence a little? (Fixes for minor typos included.)
> Something like:
> 
> "This flag initializes gl_Position to vec4(0.0, 0.0, 0.0, 1.0) at the
beginning
> of the vertex shader, and has no effect in the fragment shader."
> 
> Could you please also add another sentence like: "This flag is intended as a
> workaround for drivers which incorrectly fail to link programs if gl_Position
is
> not written."

Done.

https://codereview.appspot.com/13509043/diff/6001/src/compiler/ShHandle.h
File src/compiler/ShHandle.h (right):

https://codereview.appspot.com/13509043/diff/6001/src/compiler/ShHandle.h#new...
src/compiler/ShHandle.h:99: virtual void translate(TIntermNode* root, bool
initGLPosition) = 0;
On 2013/09/04 00:23:25, kbr1 wrote:
> Adding this boolean argument to the translate() method is ugly. Unless it's
> impossible, please add a query similar to enforcePackingRestrictions(), below.

Done.

https://codereview.appspot.com/13509043/diff/6001/src/compiler/TranslatorESSL...
File src/compiler/TranslatorESSL.cpp (right):

https://codereview.appspot.com/13509043/diff/6001/src/compiler/TranslatorESSL...
src/compiler/TranslatorESSL.cpp:31: outputESSL.enableInitGLPosition();
On 2013/09/04 00:23:25, kbr1 wrote:
> Instead of calling a method on the outputESSL object, please pass the flag as
an
> argument to the constructor, similarly to how the index clamping strategy and
> hash function are handled.

Done.

[kbr1, 2013-09-04 17:07:20]

Thanks, this looks much nicer. LGTM

https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp#...
src/compiler/Compiler.cpp:149: setInitGLPosition((compileOptions &
SH_INIT_GL_POSITION) == SH_INIT_GL_POSITION);
Could this be written more simply as setInitGLPosition(compileOptions &
SH_INIT_GL_POSITION)?

[Zhenyao Mo, 2013-09-04 17:09:09]

On 2013/09/04 17:07:20, kbr1 wrote:
> Thanks, this looks much nicer. LGTM
> 
> https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp
> File src/compiler/Compiler.cpp (right):
> 
>
https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp#...
> src/compiler/Compiler.cpp:149: setInitGLPosition((compileOptions &
> SH_INIT_GL_POSITION) == SH_INIT_GL_POSITION);
> Could this be written more simply as setInitGLPosition(compileOptions &
> SH_INIT_GL_POSITION)?

I think on windows that will cause a warning which is treated as an error.

[kbr1, 2013-09-04 17:50:39]

On 2013/09/04 17:09:09, Zhenyao Mo wrote:
> On 2013/09/04 17:07:20, kbr1 wrote:
> > Thanks, this looks much nicer. LGTM
> > 
> > https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp
> > File src/compiler/Compiler.cpp (right):
> > 
> >
>
https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp#...
> > src/compiler/Compiler.cpp:149: setInitGLPosition((compileOptions &
> > SH_INIT_GL_POSITION) == SH_INIT_GL_POSITION);
> > Could this be written more simply as setInitGLPosition(compileOptions &
> > SH_INIT_GL_POSITION)?
> 
> I think on windows that will cause a warning which is treated as an error.

OK, no problem. (What about setInitGLPosition((compileOptions &
SH_INIT_GL_POSITION) != 0)?)

[Zhenyao Mo, 2013-09-04 17:51:36]

On 2013/09/04 17:50:39, kbr1 wrote:
> On 2013/09/04 17:09:09, Zhenyao Mo wrote:
> > On 2013/09/04 17:07:20, kbr1 wrote:
> > > Thanks, this looks much nicer. LGTM
> > > 
> > >
https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp
> > > File src/compiler/Compiler.cpp (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/13509043/diff/21001/src/compiler/Compiler.cpp#...
> > > src/compiler/Compiler.cpp:149: setInitGLPosition((compileOptions &
> > > SH_INIT_GL_POSITION) == SH_INIT_GL_POSITION);
> > > Could this be written more simply as setInitGLPosition(compileOptions &
> > > SH_INIT_GL_POSITION)?
> > 
> > I think on windows that will cause a warning which is treated as an error.
> 
> OK, no problem. (What about setInitGLPosition((compileOptions &
> SH_INIT_GL_POSITION) != 0)?)

Yeah I can use this.

[Zhenyao Mo, 2013-09-05 18:33:36]

Alok, please take a look!

[Alok Priyadarshi, 2013-09-05 18:42:40]

On 2013/09/05 18:33:36, Zhenyao Mo wrote:
> Alok, please take a look!

Couple of questions:
- What happens in HLSL when vertex position is not initialized?
- I think this should not be implemented in the translator, but rather an AST
visitor. The translator should simply write whatever is in the tree. The visitor
will insert an initialization node in the tree if missing.

[Zhenyao Mo, 2013-09-05 19:14:59]

On 2013/09/05 18:42:40, Alok Priyadarshi wrote:
> On 2013/09/05 18:33:36, Zhenyao Mo wrote:
> > Alok, please take a look!
> 
> Couple of questions:
> - What happens in HLSL when vertex position is not initialized?

HLSL compiles and links fine.

> - I think this should not be implemented in the translator, but rather an AST
> visitor. The translator should simply write whatever is in the tree. The
visitor
> will insert an initialization node in the tree if missing.

Modifying AST tree is no trivia work.  I don't think the current APIs are
designed for us to make modifications at any time.  It's definitely not worth my
time.  Besides, we did a bunch of work in the translator already, like name
hashing, etc.

[Alok Priyadarshi, 2013-09-05 20:00:57]

On 2013/09/05 19:14:59, Zhenyao Mo wrote:
> On 2013/09/05 18:42:40, Alok Priyadarshi wrote:
> > On 2013/09/05 18:33:36, Zhenyao Mo wrote:
> > > Alok, please take a look!
> > 
> > Couple of questions:
> > - What happens in HLSL when vertex position is not initialized?
> 
> HLSL compiles and links fine.
> 
> > - I think this should not be implemented in the translator, but rather an
AST
> > visitor. The translator should simply write whatever is in the tree. The
> visitor
> > will insert an initialization node in the tree if missing.
> 
> Modifying AST tree is no trivia work.  I don't think the current APIs are
> designed for us to make modifications at any time.  It's definitely not worth
my
> time.  Besides, we did a bunch of work in the translator already, like name
> hashing, etc.

We modify the AST in many traversals - MapLongVariableNames for example. I do
not think it will be too difficult. In fact you already have most of the code in
this patch. I am happy to help.

The nice thing about doing this in traversal is that it keeps the code modular.
I am concerned about the output stage becoming complicated over time.

[Zhenyao Mo, 2013-09-05 20:44:00]

On 2013/09/05 20:00:57, Alok Priyadarshi wrote:
> On 2013/09/05 19:14:59, Zhenyao Mo wrote:
> > On 2013/09/05 18:42:40, Alok Priyadarshi wrote:
> > > On 2013/09/05 18:33:36, Zhenyao Mo wrote:
> > > > Alok, please take a look!
> > > 
> > > Couple of questions:
> > > - What happens in HLSL when vertex position is not initialized?
> > 
> > HLSL compiles and links fine.
> > 
> > > - I think this should not be implemented in the translator, but rather an
> AST
> > > visitor. The translator should simply write whatever is in the tree. The
> > visitor
> > > will insert an initialization node in the tree if missing.
> > 
> > Modifying AST tree is no trivia work.  I don't think the current APIs are
> > designed for us to make modifications at any time.  It's definitely not
worth
> my
> > time.  Besides, we did a bunch of work in the translator already, like name
> > hashing, etc.
> 
> We modify the AST in many traversals - MapLongVariableNames for example. I do
> not think it will be too difficult. In fact you already have most of the code
in
> this patch. I am happy to help.
> 
> The nice thing about doing this in traversal is that it keeps the code
modular.
> I am concerned about the output stage becoming complicated over time.

OK, if you are willing to help, feel free to take over this CL.

Honestly I still don't think it's worth the effort, as this is a workaround, and
should fade out once the driver fixes the bug.

As a matter of fact, I am planning to remove MapLongVaribaleNames since we
handle it through hashing instead.

[Zhenyao Mo, 2013-09-06 00:08:37]

kbr, alokp, please take another look at the new implementation.

Tested with webgl conformance tests and a few webgl demos.

[kbr1, 2013-09-06 01:02:02]

LGTM.

I'm not an expert on the IR so defer to Alok's review.

https://codereview.appspot.com/13509043/diff/35001/src/compiler/InitializeGLP...
File src/compiler/InitializeGLPosition.cpp (right):

https://codereview.appspot.com/13509043/diff/35001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:30: }
Are both of these branches tested by the WebGL conformance tests?

[Zhenyao Mo, 2013-09-06 01:34:00]

https://codereview.appspot.com/13509043/diff/35001/src/compiler/InitializeGLP...
File src/compiler/InitializeGLPosition.cpp (right):

https://codereview.appspot.com/13509043/diff/35001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:30: }
On 2013/09/06 01:02:02, kbr1 wrote:
> Are both of these branches tested by the WebGL conformance tests?

Thanks for asking.  Yeah, one path was not covered and actually there is a bug
in it.  Just updated the CL to fix it.

I sent a pull request to github/WebGL, adding a test case for the other path.

[kbr1, 2013-09-06 17:10:29]

LGTM again.

[Alok Priyadarshi, 2013-09-09 17:19:59]

minor comments. lgtm.

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
File src/compiler/InitializeGLPosition.cpp (right):

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:15: case EOpSequence: break;
nit: please follow the style-guide to format the switch statement - 2-space
indentation.

https://code.google.com/p/angleproject/wiki/CodingStandard

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:16: case EOpFunction: {
braces go on a separate line

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:28: } else {
nit: braces go on a separate line.

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:53: TIntermSequence& vec4 =
right->getSequence();
nit: I believe vec4 can be set in a better way. TIntermConstantUnion can hold an
array of ConstantUnion. You can set TType with BasicType as Float and
NominalSize as 4.

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
File src/compiler/InitializeGLPosition.h (right):

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.h:7: #ifndef
CROSSCOMPILERGLSL_INITIALIZEGLPOSITION_H_
what is CROSSCOMPILERGLSL?

[Zhenyao Mo, 2013-09-09 17:57:36]

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
File src/compiler/InitializeGLPosition.cpp (right):

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:16: case EOpFunction: {
On 2013/09/09 17:20:00, Alok Priyadarshi wrote:
> braces go on a separate line

Done.

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:28: } else {
On 2013/09/09 17:20:00, Alok Priyadarshi wrote:
> nit: braces go on a separate line.

Done.

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.cpp:53: TIntermSequence& vec4 =
right->getSequence();
On 2013/09/09 17:20:00, Alok Priyadarshi wrote:
> nit: I believe vec4 can be set in a better way. TIntermConstantUnion can hold
an
> array of ConstantUnion. You can set TType with BasicType as Float and
> NominalSize as 4.

Done.

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
File src/compiler/InitializeGLPosition.h (right):

https://codereview.appspot.com/13509043/diff/41001/src/compiler/InitializeGLP...
src/compiler/InitializeGLPosition.h:7: #ifndef
CROSSCOMPILERGLSL_INITIALIZEGLPOSITION_H_
On 2013/09/09 17:20:00, Alok Priyadarshi wrote:
> what is CROSSCOMPILERGLSL?

Done.

[Zhenyao Mo, 2013-09-09 17:59:33]

Committed patchset #8 manually as rfc75e21 (presubmit successful).