Emulate certain buil-in functions to work around driver bugs.

Emulate certain buil-in functions to work around driver bugs.

This is implemented by adding a new compile option SH_EMULATE_BUILT_IN_FUNCTIONS.  The emulated functions are names as webgl_originalName_emu so there will never be naming conflicts.

At the moment only three functions are emulated: normalize, abs, sign.  Also, the compile option will emulate all three.  However, the mechanism to emulate only a selected subset is also imeplemented.  It can be turned on easily.

ANGLEBUG=196
TEST=with this option, the failed test with abs.frag passes.
Committed: http://code.google.com/p/angleproject/source/detail?r=738

[zmo, 2011-08-18 17:49:18]

Please review.

[kbr1, 2011-08-18 21:05:06]

This is good work in general, but a couple of comments about the code structure.

http://codereview.appspot.com/4916043/diff/1/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/4916043/diff/1/include/GLSLANG/ShaderLang.h#new...
include/GLSLANG/ShaderLang.h:86: SH_EMULATE_BUILT_IN_FUNCTIONS = 0x0100
Please add a comment indicating that this is needed only as a workaround for
certain OpenGL driver bugs.

http://codereview.appspot.com/4916043/diff/1/samples/translator/translator.cpp
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/4916043/diff/1/samples/translator/translator.cp...
samples/translator/translator.cpp:188: "       -e       : emulate certain
built-in functions\n"
Please add "(workaround for driver bugs)"

http://codereview.appspot.com/4916043/diff/1/src/compiler/BuiltInFunctionEmul...
File src/compiler/BuiltInFunctionEmulator.h (right):

http://codereview.appspot.com/4916043/diff/1/src/compiler/BuiltInFunctionEmul...
src/compiler/BuiltInFunctionEmulator.h:25: class BuiltInFunctionEmulator {
Please add a big comment at the top of this class indicating that it's only a
workaround for OpenGL driver bugs, and isn't needed in general.

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

http://codereview.appspot.com/4916043/diff/1/src/compiler/OutputGLSLBase.cpp#...
src/compiler/OutputGLSLBase.cpp:393: writeTriplet(visit, preString.c_str(),
NULL, postString.c_str());
It would be cleaner if the BuiltInFunctionEmulator actually rewrote the AST to
turn the TIntermUnary into a function call -- that way this code wouldn't have
to know about the emulation. That might involve more code, but would keep the
change more localized. What do you think?

http://codereview.appspot.com/4916043/diff/1/src/compiler/intermediate.h
File src/compiler/intermediate.h (right):

http://codereview.appspot.com/4916043/diff/1/src/compiler/intermediate.h#newc...
src/compiler/intermediate.h:424: bool getUseEmulatedFunction() { return
useEmulatedFunction; }
It would be nicer to avoid changing this class for this purpose.

[zmo, 2011-08-19 17:20:06]

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

http://codereview.appspot.com/4916043/diff/1/src/compiler/OutputGLSLBase.cpp#...
src/compiler/OutputGLSLBase.cpp:393: writeTriplet(visit, preString.c_str(),
NULL, postString.c_str());
On 2011/08/18 21:05:06, kbr1 wrote:
> It would be cleaner if the BuiltInFunctionEmulator actually rewrote the AST to
> turn the TIntermUnary into a function call -- that way this code wouldn't have
> to know about the emulation. That might involve more code, but would keep the
> change more localized. What do you think?

Good point.  I'll have a look at how much work this involves.

[dgkoch, 2011-08-22 17:31:30]

http://codereview.appspot.com/4916043/diff/1/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/4916043/diff/1/src/build_angle.gyp#newcode23
src/build_angle.gyp:23: 'compiler/BuiltInFunctionEmulator.h',
please add these to translator_common.vcproj as well.

[zmo, 2011-08-24 00:38:12]

Revised.  Please review again.

http://codereview.appspot.com/4916043/diff/1/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/4916043/diff/1/include/GLSLANG/ShaderLang.h#new...
include/GLSLANG/ShaderLang.h:86: SH_EMULATE_BUILT_IN_FUNCTIONS = 0x0100
On 2011/08/18 21:05:06, kbr1 wrote:
> Please add a comment indicating that this is needed only as a workaround for
> certain OpenGL driver bugs.

Done.

http://codereview.appspot.com/4916043/diff/1/samples/translator/translator.cpp
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/4916043/diff/1/samples/translator/translator.cp...
samples/translator/translator.cpp:188: "       -e       : emulate certain
built-in functions\n"
On 2011/08/18 21:05:06, kbr1 wrote:
> Please add "(workaround for driver bugs)"

Done.

http://codereview.appspot.com/4916043/diff/1/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/4916043/diff/1/src/build_angle.gyp#newcode23
src/build_angle.gyp:23: 'compiler/BuiltInFunctionEmulator.h',
On 2011/08/22 17:31:30, dgkoch wrote:
> please add these to translator_common.vcproj as well.

Done.

http://codereview.appspot.com/4916043/diff/1/src/compiler/BuiltInFunctionEmul...
File src/compiler/BuiltInFunctionEmulator.h (right):

http://codereview.appspot.com/4916043/diff/1/src/compiler/BuiltInFunctionEmul...
src/compiler/BuiltInFunctionEmulator.h:25: class BuiltInFunctionEmulator {
On 2011/08/18 21:05:06, kbr1 wrote:
> Please add a big comment at the top of this class indicating that it's only a
> workaround for OpenGL driver bugs, and isn't needed in general.

Done.

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

http://codereview.appspot.com/4916043/diff/1/src/compiler/OutputGLSLBase.cpp#...
src/compiler/OutputGLSLBase.cpp:393: writeTriplet(visit, preString.c_str(),
NULL, postString.c_str());
On 2011/08/18 21:05:06, kbr1 wrote:
> It would be cleaner if the BuiltInFunctionEmulator actually rewrote the AST to
> turn the TIntermUnary into a function call -- that way this code wouldn't have
> to know about the emulation. That might involve more code, but would keep the
> change more localized. What do you think?

Per our offline discussion, this will be done in another CL, as this CL is big
enough already.

http://codereview.appspot.com/4916043/diff/1/src/compiler/intermediate.h
File src/compiler/intermediate.h (right):

http://codereview.appspot.com/4916043/diff/1/src/compiler/intermediate.h#newc...
src/compiler/intermediate.h:424: bool getUseEmulatedFunction() { return
useEmulatedFunction; }
On 2011/08/18 21:05:06, kbr1 wrote:
> It would be nicer to avoid changing this class for this purpose.

We either need to mark this one to be replaced by a function node (once we
implement the AST modification method), or mark it to use emulated function name
in the output phase.  Adding a new member seems unavoidable.  :(

[kbr1, 2011-08-24 01:02:26]

LGTM. Thanks for persisting on this. Per our offline discussion, it would be
great to add functionality to the AST to make editing it easier, and eventually
rephrase this change in terms of that functionality. However, I agree with you
that that should be done separately.