Refactored the way symbol tables are initialized and stored. This was done in...

Refactored the way symbol tables are initialized and stored. This was done in response to the addition of EShSpec.
Symbol table entries depend on three things - language, spec (not now but may eventually), and built-in resources.
We used to build two global symbol-tables - one for each language. During each compile, one of the symbol table was
copied and resource-specific stuff was added. I have moved the symbol table to TCompiler that gets initilized when
compiler is created and reused for each compile. This makes it much cleaner and extensible in case a spec requires
special entries to be added to the symbol table.

PS: Sorry for the long CL, but all of it needed to be done in one CL. I have verified that everything still compiles
and passes all conformance tests.

[kbr1, 2010-07-22 23:01:15]

http://codereview.appspot.com/1864044/diff/1/6
File src/compiler/Initialize.cpp (right):

http://codereview.appspot.com/1864044/diff/1/6#newcode22
src/compiler/Initialize.cpp:22: TString BuiltInFunctionsCommon()
If this isn't exported in the header it should be static. This applies to all of
the top level functions introduced here.

http://codereview.appspot.com/1864044/diff/1/6#newcode503
src/compiler/Initialize.cpp:503: break;
If I'm reading this review correctly then the builtins are being reinitialized
for each compilation, which seems like an awful lot of string concatenation to
be doing each time. If this is the case, can you try to figure out a way to
cache the results?

http://codereview.appspot.com/1864044/diff/1/15
File src/libGLESv2/Shader.cpp (right):

http://codereview.appspot.com/1864044/diff/1/15#newcode47
src/libGLESv2/Shader.cpp:47: mFragmentCompiler =
ShConstructCompiler(EShLangFragment, EShSpecGLES2, &resources);
This is passing a pointer to an on-stack variable. Is a copy made internally? It
looks risky regardless.

[Alok Priyadarshi, 2010-07-23 16:16:28]

http://codereview.appspot.com/1864044/diff/1/6
File src/compiler/Initialize.cpp (right):

http://codereview.appspot.com/1864044/diff/1/6#newcode22
src/compiler/Initialize.cpp:22: TString BuiltInFunctionsCommon()
On 2010/07/22 23:01:15, kbr1 wrote:
> If this isn't exported in the header it should be static. This applies to all
of
> the top level functions introduced here.
> 

Done.

http://codereview.appspot.com/1864044/diff/1/6#newcode503
src/compiler/Initialize.cpp:503: break;
Built-ins are initialized only once per compiler. The built-in strings are used
to build a symbol table that gets cached in the compiler. You can reuse the
compiler to do multiple compiles (ANGLE already does that). I have a CL for
chromium to make this change. I will send it as soon as I have submitted this
one.

BTW this is an improvement from the original - you do not need to copy the
symbol table which can be expensive.

http://codereview.appspot.com/1864044/diff/1/15
File src/libGLESv2/Shader.cpp (right):

http://codereview.appspot.com/1864044/diff/1/15#newcode47
src/libGLESv2/Shader.cpp:47: mFragmentCompiler =
ShConstructCompiler(EShLangFragment, EShSpecGLES2, &resources);
Yes I was making a copy in the compiler, but I realized that is not necessary.
Resources are needed only when building the symbol table which happens in
ShConstructCompiler. I have reverted the changes to copy TBuiltInResource.

[kbr1, 2010-07-23 18:34:10]

This generally looks good to me but I haven't studied how the compiler and
symbol table work in enough depth to provide a good review of the larger
changes. In particular the "TSymbolTable symbolTable" definition in TCompiler
seems a little sketchy. Is it immutable, or is it guaranteed to be restored to
its original state? If it's immutable, it should probably be called
"builtInSymbolTable"; otherwise, there should be assertions around its being
restored to its original state at the end of each compile, successful or not.

I'll defer to Daniel for the more thorough review.

[Alok Priyadarshi, 2010-07-23 18:54:20]

Hope this helps. My CL mainly moves the per-process symbol-table to the
compiler. This eliminates copying the symbol-table for each compile. It also
eliminates adding resource-specific symbols to the symbol-table for each
compile. It is less confusing and extensible in case we cannot share the
symbol-table between GLES2 and WebGL in future.

http://codereview.appspot.com/1864044/diff/6001/7006
File src/compiler/ShaderLang.cpp (left):

http://codereview.appspot.com/1864044/diff/6001/7006#oldcode239
src/compiler/ShaderLang.cpp:239: TSymbolTable
symbolTable(*SymbolTables[compiler->getLanguage()]);
Note that we had to copy symbol-table at the beginning of each compilation
because this symbol-table was per process and hence not thread-safe.

http://codereview.appspot.com/1864044/diff/6001/7006
File src/compiler/ShaderLang.cpp (right):

http://codereview.appspot.com/1864044/diff/6001/7006#newcode194
src/compiler/ShaderLang.cpp:194: if (!symbolTable.atGlobalLevel())
The symbol-table cached in compiler only contains built-in symbols except during
compilation. At the beginning of the compilation we push a level and add
shader-specific symbols. Here is an assertion to verify that.

http://codereview.appspot.com/1864044/diff/6001/7006#newcode239
src/compiler/ShaderLang.cpp:239: while (!symbolTable.atBuiltInLevel())
This is where we pop all shader-specific symbols to restore the symbol-table to
contain only built-in symbols.

[dgkoch, 2010-07-26 13:28:30]

Look reasonable, other than my specific comments.

Daniel

http://codereview.appspot.com/1864044/diff/6001/7002
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/1864044/diff/6001/7002#newcode77
samples/translator/translator.cpp:77: switch (FindLanguage(argv[0])) {
You probably want to use 'lang' here instead of calling FindLanguage(argv[0])
again.

http://codereview.appspot.com/1864044/diff/6001/7003
File src/compiler/Initialize.cpp (left):

http://codereview.appspot.com/1864044/diff/6001/7003#oldcode585
src/compiler/Initialize.cpp:585: case EShLangFragment:
I'd prefer if the dFdx/dFdy/fwidth operators were left here (although still
commented out) as the OES_standard_derivatives extension is something that would
potentially be useful for an ES2/webgl implementation.

http://codereview.appspot.com/1864044/diff/6001/7003
File src/compiler/Initialize.cpp (right):

http://codereview.appspot.com/1864044/diff/6001/7003#newcode361
src/compiler/Initialize.cpp:361: s.append(TString("vec4 ftransform();"));
should probably comment this line out.  ftransform() isn't valid for ES2/webgl

[Alok Priyadarshi, 2010-07-26 18:08:52]

http://codereview.appspot.com/1864044/diff/6001/7002
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/1864044/diff/6001/7002#newcode77
samples/translator/translator.cpp:77: switch (FindLanguage(argv[0])) {
I removed lang variable instead. It was not being used anywhere.

http://codereview.appspot.com/1864044/diff/6001/7003
File src/compiler/Initialize.cpp (left):

http://codereview.appspot.com/1864044/diff/6001/7003#oldcode585
src/compiler/Initialize.cpp:585: case EShLangFragment:
On 2010/07/26 13:28:30, dgkoch wrote:
> I'd prefer if the dFdx/dFdy/fwidth operators were left here (although still
> commented out) as the OES_standard_derivatives extension is something that
would
> potentially be useful for an ES2/webgl implementation.

Done.

http://codereview.appspot.com/1864044/diff/6001/7003
File src/compiler/Initialize.cpp (right):

http://codereview.appspot.com/1864044/diff/6001/7003#newcode361
src/compiler/Initialize.cpp:361: s.append(TString("vec4 ftransform();"));
On 2010/07/26 13:28:30, dgkoch wrote:
> should probably comment this line out.  ftransform() isn't valid for ES2/webgl

Done.