Implement ES2 backend for Angle translator.

Implement ES2 backend for Angle translator.

With this CL, we have the option to select a code output backend: GLSL, GLSL ES, or HLSL.

Note that we always emit the highest supported float precision for fragment shader due to anglebug 168.  Although this is a temporary solution, it's not against GLSL ES spec, because it's ok for implementation to upgrade precision.

Tested with WebGL conformance test suite, GLES2 conformance test suite (only failed 2/1198), and a few webgl demos, including worlds of webgl, aquarium, etc.

anglebug=81
test=translator emitting correct GLSL ES code when ES2 backend is selected.
Committed: http://code.google.com/p/angleproject/source/detail?r=687

[Zhenyao Mo, 2011-06-09 20:46:07]

Please review.

[kbr1, 2011-06-10 01:03:33]

This looks excellent. This refactoring is very important, as it finally allows
all shaders to run through the translator, even when running on a native GLES2
implementation. Despite our offline discussion I would definitely support
committing this before Issue 168 is fixed.

Note, you need to update BUILD_REVISION in src/common/version.h.

http://codereview.appspot.com/4550129/diff/1001/src/compiler/TranslatorESSL.cpp
File src/compiler/TranslatorESSL.cpp (right):

http://codereview.appspot.com/4550129/diff/1001/src/compiler/TranslatorESSL.c...
src/compiler/TranslatorESSL.cpp:11: static void writeExtensionBehavior(const
TExtensionBehavior& extensionBehavior,
Why not make this a private member function? Then it wouldn't need any
arguments.

http://codereview.appspot.com/4550129/diff/1001/src/compiler/TranslatorESSL.c...
src/compiler/TranslatorESSL.cpp:40: }
As we've discussed offline I think it would be nicer to fix Issue 168 before
this one, but after reviewing this patch more thoroughly I would definitely
support committing this first and working on that afterward.

[kbr1, 2011-06-10 01:07:23]

http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.cpp
File src/compiler/OutputGLSLBase.cpp (right):

http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.c...
src/compiler/OutputGLSLBase.cpp:115: out << getTypeName(*fieldType) << " " <<
fieldType->getFieldName();
Some change will (at least eventually) be needed here or somewhere related in
order to support precision qualifiers for fields in structs.

[Zhenyao Mo, 2011-06-10 01:49:13]

Revised.  Please have another quick look.

Also, Alok, Nicolas, do you have any further comments besides Ken's review?

http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.cpp
File src/compiler/OutputGLSLBase.cpp (right):

http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.c...
src/compiler/OutputGLSLBase.cpp:115: out << getTypeName(*fieldType) << " " <<
fieldType->getFieldName();
On 2011/06/10 01:07:23, kbr1 wrote:
> Some change will (at least eventually) be needed here or somewhere related in
> order to support precision qualifiers for fields in structs.

Done.

http://codereview.appspot.com/4550129/diff/1001/src/compiler/TranslatorESSL.cpp
File src/compiler/TranslatorESSL.cpp (right):

http://codereview.appspot.com/4550129/diff/1001/src/compiler/TranslatorESSL.c...
src/compiler/TranslatorESSL.cpp:11: static void writeExtensionBehavior(const
TExtensionBehavior& extensionBehavior,
On 2011/06/10 01:03:33, kbr1 wrote:
> Why not make this a private member function? Then it wouldn't need any
> arguments.

This is following the pattern of TranslatorGLSL, but you are right, we should
actually make this a member.  Done.

[kbr1, 2011-06-10 02:03:29]

LGTM

http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.cpp
File src/compiler/OutputGLSLBase.cpp (right):

http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.c...
src/compiler/OutputGLSLBase.cpp:115: out << getTypeName(*fieldType) << " " <<
fieldType->getFieldName();
On 2011/06/10 01:49:13, Zhenyao Mo wrote:
> On 2011/06/10 01:07:23, kbr1 wrote:
> > Some change will (at least eventually) be needed here or somewhere related
in
> > order to support precision qualifiers for fields in structs.
> 
> Done.

Has this change been tested?

[Zhenyao Mo, 2011-06-10 04:26:52]

On 2011/06/10 02:03:29, kbr1 wrote:
> LGTM
> 
>
http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.cpp
> File src/compiler/OutputGLSLBase.cpp (right):
> 
>
http://codereview.appspot.com/4550129/diff/1001/src/compiler/OutputGLSLBase.c...
> src/compiler/OutputGLSLBase.cpp:115: out << getTypeName(*fieldType) << " " <<
> fieldType->getFieldName();
> On 2011/06/10 01:49:13, Zhenyao Mo wrote:
> > On 2011/06/10 01:07:23, kbr1 wrote:
> > > Some change will (at least eventually) be needed here or somewhere related
> in
> > > order to support precision qualifiers for fields in structs.
> > 
> > Done.
> 
> Has this change been tested?

Yes, only that now the precision in struct fields are lost in translation, so
the code has no effects.  It will be fixed in bug 168.

[Alok Priyadarshi, 2011-06-10 05:53:25]

http://codereview.appspot.com/4550129/diff/1002/samples/translator/translator...
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/4550129/diff/1002/samples/translator/translator...
samples/translator/translator.cpp:91: vertexCompiler = ShConstructCompiler(
could you make this a command-line flag?

http://codereview.appspot.com/4550129/diff/1002/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/4550129/diff/1002/src/build_angle.gyp#newcode122
src/build_angle.gyp:122: 'target_name': 'translator_hlsl',
Now that you can control the output with a paramater, we may not need two
different targets for glsl and hlsl.

http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h
File src/compiler/OutputGLSLBase.h (right):

http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h...
src/compiler/OutputGLSLBase.h:16: class TOutputGLSLBase : public
TIntermTraverser
I don't quite like the fact that so much code was moved around for such a
trivial change. I think this could be simply achieved by adding an extra
parameter for output type to TOutputGLSL itself.

[kbr1, 2011-06-10 13:42:05]

http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h
File src/compiler/OutputGLSLBase.h (right):

http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h...
src/compiler/OutputGLSLBase.h:16: class TOutputGLSLBase : public
TIntermTraverser
On 2011/06/10 05:53:26, Alok Priyadarshi wrote:
> I don't quite like the fact that so much code was moved around for such a
> trivial change. I think this could be simply achieved by adding an extra
> parameter for output type to TOutputGLSL itself.

It could certainly be done that way, but would make maintaining the code harder
in the long run. The refactoring Mo has done makes explicit the differences
between GLSL and ESSL, and I think we should do it.

I applied part of the patch for this CL and manually diffed TOutputGLSLBase with
the current TOutputGLSL to verify that only the desired changes were made during
this refactoring.

[Alok Priyadarshi, 2011-06-10 15:42:30]

On 2011/06/10 13:42:05, kbr1 wrote:
> http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h
> File src/compiler/OutputGLSLBase.h (right):
> 
>
http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h...
> src/compiler/OutputGLSLBase.h:16: class TOutputGLSLBase : public
> TIntermTraverser
> On 2011/06/10 05:53:26, Alok Priyadarshi wrote:
> > I don't quite like the fact that so much code was moved around for such a
> > trivial change. I think this could be simply achieved by adding an extra
> > parameter for output type to TOutputGLSL itself.
> 
> It could certainly be done that way, but would make maintaining the code
harder
> in the long run. The refactoring Mo has done makes explicit the differences
> between GLSL and ESSL, and I think we should do it.
> 

My suggestion certainly falls under personal preferences category. I think it is
OK to split TranslatorESSL, but may be not TOutputESSL. If I were maintaining
this code I would keep all GLSL output code together because the differences are
so slight.

But I think we should certainly consider consolidating all translator related
build targets (translator_common, translator_glsl, and translator_hlsl) into
shader_translater. This can be done in a separate CL.

[zmo, 2011-06-10 16:43:22]

On 2011/06/10 15:42:30, Alok Priyadarshi wrote:
> On 2011/06/10 13:42:05, kbr1 wrote:
> >
http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h
> > File src/compiler/OutputGLSLBase.h (right):
> > 
> >
>
http://codereview.appspot.com/4550129/diff/1002/src/compiler/OutputGLSLBase.h...
> > src/compiler/OutputGLSLBase.h:16: class TOutputGLSLBase : public
> > TIntermTraverser
> > On 2011/06/10 05:53:26, Alok Priyadarshi wrote:
> > > I don't quite like the fact that so much code was moved around for such a
> > > trivial change. I think this could be simply achieved by adding an extra
> > > parameter for output type to TOutputGLSL itself.
> > 
> > It could certainly be done that way, but would make maintaining the code
> harder
> > in the long run. The refactoring Mo has done makes explicit the differences
> > between GLSL and ESSL, and I think we should do it.
> > 
> 
> My suggestion certainly falls under personal preferences category. I think it
is
> OK to split TranslatorESSL, but may be not TOutputESSL. If I were maintaining
> this code I would keep all GLSL output code together because the differences
are
> so slight.

I agree the differences are slight at the moment, but those differences are
essential.  Splitting the ESSL and GLSL makes the differences clear and in one
place, thus making future adding/removing differences easier, as we can expect
the differences between ESSL/GLSL might change.

Also, if we miss any differences in the ESSL/GLSL (I hope not), it is easy for
whoever maintain this code to see directly if something is missing.

> 
> But I think we should certainly consider consolidating all translator related
> build targets (translator_common, translator_glsl, and translator_hlsl) into
> shader_translater. This can be done in a separate CL.

I think the current situation is that translator_hlsl is part of the Angle DLL,
but not part of the Shader Translator.  translator_glsl is part of the Shader
Translator, but not part of the Angle DLL.  In that case, we do need to keep
those targets separate.

[Zhenyao Mo, 2011-06-10 18:05:39]

Added commandline option for samples/translator output backend selection.  Alok,
please have a quick look again.

http://codereview.appspot.com/4550129/diff/1002/samples/translator/translator...
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/4550129/diff/1002/samples/translator/translator...
samples/translator/translator.cpp:91: vertexCompiler = ShConstructCompiler(
On 2011/06/10 05:53:26, Alok Priyadarshi wrote:
> could you make this a command-line flag?

Done.

[Alok Priyadarshi, 2011-06-10 18:12:16]

On 2011/06/10 18:05:39, Zhenyao Mo wrote:
> Added commandline option for samples/translator output backend selection. 
Alok,
> please have a quick look again.
> 
>
http://codereview.appspot.com/4550129/diff/1002/samples/translator/translator...
> File samples/translator/translator.cpp (right):
> 
>
http://codereview.appspot.com/4550129/diff/1002/samples/translator/translator...
> samples/translator/translator.cpp:91: vertexCompiler = ShConstructCompiler(
> On 2011/06/10 05:53:26, Alok Priyadarshi wrote:
> > could you make this a command-line flag?
> 
> Done.

lgtm