Implement shader identifier name mapping.

Implement shader identifier name mapping.

The name mapping happens when an identifier is longer than 32 characters.  The name mapping is behind a flag, so it won't happen by default.  Also, functions to query the mapped names are added.

The purpose of this CL is for the drivers that can't handle long names.  For example, linux NVIDIA driver can't handle 256 character name, whereas WebGL spec requires that.

This CL also fixes the issue that some of the TIntermSymbols' ids are 0s.

ANGLEBUG=144
TEST=test manually with shaders with long identifier names.
Committed: http://code.google.com/p/angleproject/source/detail?r=619

[Zhenyao Mo, 2011-04-21 00:36:08]

Please review.

[kbr1, 2011-04-21 01:07:45]

On 2011/04/21 00:36:08, Zhenyao Mo wrote:
> Please review.

include/GLSLANG/ShaderLang.h needs to be updated with the API changes to
ShGetActiveAttrib and ShGetActiveUniform, and the API version updated.

I'm hoping Alok can provide feedback on the implementation. I would suggest
asking Nicolas at TransGaming for review as well.

[Zhenyao Mo, 2011-04-21 01:20:35]

Oops, forget to upload ShaderLang.h.  Uploaded.

Alok and Nicolas, please review.

[Alok Priyadarshi, 2011-04-21 15:54:25]

http://codereview.appspot.com/4428058/diff/6001/src/compiler/MapLongVariableN...
File src/compiler/MapLongVariableNames.cpp (right):

http://codereview.appspot.com/4428058/diff/6001/src/compiler/MapLongVariableN...
src/compiler/MapLongVariableNames.cpp:116: if (list[ii].name ==
originalName.c_str()) {
I do not see how this will work. For a shader like this:

struct Foo {
  vec4 color[1];
};
struct Bar {
  Foo foo;
};
uniform Bar bar[2];
void main() {
  gl_FragColor = bar[0].foo.color[0] + bar[1].foo.color[0];
}

The names of the two uniform variables will be: bar[0].foo.color[0] and
bar[1].foo.color[0]. The string matching will never succeed because you will
comparing it with "bar".

Since ShGetActiveAttrib/Uniform returns decorated names while the mapped name is
not decorated, I would suggest keeping this new functionality separate. I would
create a new function that returns a table of all mapped variables.

[Zhenyao Mo, 2011-04-21 17:07:29]

On 2011/04/21 15:54:25, Alok Priyadarshi wrote:
>
http://codereview.appspot.com/4428058/diff/6001/src/compiler/MapLongVariableN...
> File src/compiler/MapLongVariableNames.cpp (right):
> 
>
http://codereview.appspot.com/4428058/diff/6001/src/compiler/MapLongVariableN...
> src/compiler/MapLongVariableNames.cpp:116: if (list[ii].name ==
> originalName.c_str()) {
> I do not see how this will work. For a shader like this:
> 
> struct Foo {
>   vec4 color[1];
> };
> struct Bar {
>   Foo foo;
> };
> uniform Bar bar[2];
> void main() {
>   gl_FragColor = bar[0].foo.color[0] + bar[1].foo.color[0];
> }
> 
> The names of the two uniform variables will be: bar[0].foo.color[0] and
> bar[1].foo.color[0]. The string matching will never succeed because you will
> comparing it with "bar".

I see.  Will fix this.

> 
> Since ShGetActiveAttrib/Uniform returns decorated names while the mapped name
is
> not decorated, I would suggest keeping this new functionality separate. I
would
> create a new function that returns a table of all mapped variables.

Per discussion with Alok offline, we will do name mapping while parsing and
validating names instead of a separate pass.  Then when we construct the
attribs/uniforms name table, we could construct original/mapped names at the
same time.

[Zhenyao Mo, 2011-04-21 23:39:05]

Revised, please review again.

Changes from previous CL: 
1) Cache mapped name in TSymbol so we don't do it multiple times.
2) Do name mapping first, then build the attribs/uniforms table; now we handle
nested structure variable name mapping correctly.
3) After more digging, I realized it is much easier to implement name mapping as
a separate pass instead of as part of the compilation pass, the reason is naming
mapping requires symbol table lookup, so if the symbol table is not complete,
then once we don't find a symbol in the table, we don't know if it's because
wrong syntax, or because the symbol is a local variable.

[Alok Priyadarshi, 2011-04-22 19:36:49]

http://codereview.appspot.com/4428058/diff/6003/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/Compiler.cpp#new...
src/compiler/Compiler.cpp:151: // mapLongVariableNames() needs to be called
before collectAttribsUniforms().
nit: 80 cols

http://codereview.appspot.com/4428058/diff/6003/src/compiler/Compiler.cpp#new...
src/compiler/Compiler.cpp:151: // mapLongVariableNames() needs to be called
before collectAttribsUniforms().
Please add a small description why it needs to be called before.

http://codereview.appspot.com/4428058/diff/6003/src/compiler/MapLongVariableN...
File src/compiler/MapLongVariableNames.cpp (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/MapLongVariableN...
src/compiler/MapLongVariableNames.cpp:20: TSymbol* symbol =
mSymbolTable.find(symbolNode->getSymbol());
why are you changing TSymbol? You could just set the mapped name on
TIntermSymbol. Am I missing something?

http://codereview.appspot.com/4428058/diff/6003/src/compiler/SymbolTable.h
File src/compiler/SymbolTable.h (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/SymbolTable.h#ne...
src/compiler/SymbolTable.h:59: const TString& getMappedName()
I do not see why you need to change TSymbol. This does not even appear in the
AST. I think you could just TIntermSymbol::remapName() it should be sufficient.

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.cpp
File src/compiler/VariableInfo.cpp (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.cpp...
src/compiler/VariableInfo.cpp:139:
CollectAttribsUniforms::CollectAttribsUniforms(TSymbolTable& symbolTable,
If you set the mapped name on TIntermSymbol, then you do not need to access
TSymbolTable here.

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.h
File src/compiler/VariableInfo.h (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.h#n...
src/compiler/VariableInfo.h:27: CollectAttribsUniforms(TSymbolTable&
symbolTable,
should this be const?

[Zhenyao Mo, 2011-04-22 20:16:54]

http://codereview.appspot.com/4428058/diff/6003/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/Compiler.cpp#new...
src/compiler/Compiler.cpp:151: // mapLongVariableNames() needs to be called
before collectAttribsUniforms().
On 2011/04/22 19:36:49, Alok Priyadarshi wrote:
> Please add a small description why it needs to be called before.

Will do

http://codereview.appspot.com/4428058/diff/6003/src/compiler/MapLongVariableN...
File src/compiler/MapLongVariableNames.cpp (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/MapLongVariableN...
src/compiler/MapLongVariableNames.cpp:20: TSymbol* symbol =
mSymbolTable.find(symbolNode->getSymbol());
On 2011/04/22 19:36:49, Alok Priyadarshi wrote:
> why are you changing TSymbol? You could just set the mapped name on
> TIntermSymbol. Am I missing something?

The same identifier has multiple TIntermSymbol nodes with various IDs.  So I
can't use it to generate the mapped name (which is webgl_id_xxx).  That's why I
have to go back to TSymbolTable to get a unique ID.

http://codereview.appspot.com/4428058/diff/6003/src/compiler/SymbolTable.h
File src/compiler/SymbolTable.h (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/SymbolTable.h#ne...
src/compiler/SymbolTable.h:59: const TString& getMappedName()
On 2011/04/22 19:36:49, Alok Priyadarshi wrote:
> I do not see why you need to change TSymbol. This does not even appear in the
> AST. I think you could just TIntermSymbol::remapName() it should be
sufficient.

Explained above.

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.cpp
File src/compiler/VariableInfo.cpp (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.cpp...
src/compiler/VariableInfo.cpp:139:
CollectAttribsUniforms::CollectAttribsUniforms(TSymbolTable& symbolTable,
On 2011/04/22 19:36:49, Alok Priyadarshi wrote:
> If you set the mapped name on TIntermSymbol, then you do not need to access
> TSymbolTable here.

Explained above.

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.h
File src/compiler/VariableInfo.h (right):

http://codereview.appspot.com/4428058/diff/6003/src/compiler/VariableInfo.h#n...
src/compiler/VariableInfo.h:27: CollectAttribsUniforms(TSymbolTable&
symbolTable,
On 2011/04/22 19:36:49, Alok Priyadarshi wrote:
> should this be const?

Since getMappedName compute and save the mapped name the first time it's called,
so it's not const.

[Zhenyao Mo, 2011-04-22 22:43:49]

Fixed the issue in the original code that some TIntermSymbols' ids are zero.  So
now we don't have to go back to the TSymbolTable for a valid unique ID.

Please review again.

[Alok Priyadarshi, 2011-04-22 23:25:39]

lgtm

http://codereview.appspot.com/4428058/diff/12002/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/4428058/diff/12002/src/compiler/ParseHelper.cpp...
src/compiler/ParseHelper.cpp:899: if (voidErrorCheck(line, identifier, type))
do you need to delete variable here too?

http://codereview.appspot.com/4428058/diff/12002/src/compiler/glslang_tab.cpp
File src/compiler/glslang_tab.cpp (right):

http://codereview.appspot.com/4428058/diff/12002/src/compiler/glslang_tab.cpp...
src/compiler/glslang_tab.cpp:3385: TVariable* variable;
initialize?

[Zhenyao Mo, 2011-04-23 00:44:31]

Upload the final CL, for the records.

http://codereview.appspot.com/4428058/diff/12002/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/4428058/diff/12002/src/compiler/ParseHelper.cpp...
src/compiler/ParseHelper.cpp:899: if (voidErrorCheck(line, identifier, type))
On 2011/04/22 23:25:39, Alok Priyadarshi wrote:
> do you need to delete variable here too?

I don't think so.  The variable is in the table, so will get deleted upon table
destruction.

http://codereview.appspot.com/4428058/diff/12002/src/compiler/glslang_tab.cpp
File src/compiler/glslang_tab.cpp (right):

http://codereview.appspot.com/4428058/diff/12002/src/compiler/glslang_tab.cpp...
src/compiler/glslang_tab.cpp:3385: TVariable* variable;
On 2011/04/22 23:25:39, Alok Priyadarshi wrote:
> initialize?

Done.

[kbr1, 2011-04-23 01:08:39]

The changes to src/compiler/glslang_tab.cpp are problematic, because that file
is autogenerated by flex/bison. The right way to make those changes would be to
modify either the glslang.l or glslang.y file and re-run flex and bison to
produce the parser.

[Zhenyao Mo, 2011-04-23 01:25:13]

Please review again.

[kbr1, 2011-04-23 01:28:03]

Fantastic. LGTM