Implemented validation for loop and indexing limitations specified by GLSL ES...

Implemented validation for loop and indexing limitations specified by GLSL ES spec 1.0 Appendix A Section 4 and 5.
A couple of things to note:
- This CL only validates the "form" of loop and indexing. It does not detect number-of-iterations or out-of-bound access. This will require more involved analysis/heuristics.
- I haved combined SH_VALIDATE_CONTROL_FLOW and SH_VALIDATE_INDEXING into one flag - SH_VALIDATE_LOOP_INDEXING. Validating both together is much easier.
BUG=48

Committed: http://code.google.com/p/angleproject/source/detail?r=491

[dgkoch, 2010-11-22 04:37:01]

A couple comments, but other than that, I don't have much to add.

Daniel

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

http://codereview.appspot.com/3225041/diff/2001/include/GLSLANG/ShaderLang.h#...
include/GLSLANG/ShaderLang.h:20: #define SH_VERSION 102
should this be bumped for the ShCompileOption enum changes?  (not sure given
that they haven't been used yet).

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
File src/compiler/ValidateLimitations.cpp (right):

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:101: // body of the loop? It is not very
clear from the spec.
p106 - last point of section 4 - "the loop index is not statically assigned to…"
I would think that increment/decrement would be consider to be assigning to the
loop index and thus not allowed.

[kbr1, 2010-11-22 23:33:51]

This mostly looks very good. Only a few minor stylistic comments.

Have you run any WebGL content through this patch? I would like to ensure that
we don't accidentally introduce regressions. Of course if content is not
conformant then it needs to be updated. Also, we should coordinate with gman /
zmo to find out the status of shader tests in the WebGL test suite, so that we
have both positive and negative tests for this validation.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
File src/compiler/ValidateLimitations.cpp (right):

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:101: // body of the loop? It is not very
clear from the spec.
On 2010/11/22 04:37:01, dgkoch wrote:
> p106 - last point of section 4 - "the loop index is not statically assigned
to…"
> I would think that increment/decrement would be consider to be assigning to
the
> loop index and thus not allowed.

Yes, I also think that's the correct interpretation.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:411: valid = false;
Could break here, or just return false.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:445: valid = false;
Why not just return false?

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:454: valid = false;
Same here. It seems safer to skip the above work if the first checks failed.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
File src/compiler/ValidateLimitations.h (right):

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.h:13: struct TIndex {
Should this not have a default constructor so that the id doesn't contain
garbage initially?

[Alok Priyadarshi, 2010-11-23 20:42:01]

With SH_VALIDATE_LOOP_INDEXING flag CTS passes!
All WebGL demos I have tried work!
A few of the WebGL conformance tests fail, which I am looking at whether they
are related to this CL.

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

http://codereview.appspot.com/3225041/diff/2001/include/GLSLANG/ShaderLang.h#...
include/GLSLANG/ShaderLang.h:20: #define SH_VERSION 102
On 2010/11/22 04:37:01, dgkoch wrote:
> should this be bumped for the ShCompileOption enum changes?  (not sure given
> that they haven't been used yet).

Done.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
File src/compiler/ValidateLimitations.cpp (right):

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:101: // body of the loop? It is not very
clear from the spec.
On 2010/11/22 23:33:52, kbr1 wrote:
> On 2010/11/22 04:37:01, dgkoch wrote:
> > p106 - last point of section 4 - "the loop index is not statically assigned
> to…"
> > I would think that increment/decrement would be consider to be assigning to
> the
> > loop index and thus not allowed.
> 
> Yes, I also think that's the correct interpretation.

Done.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:411: valid = false;
On 2010/11/22 23:33:52, kbr1 wrote:
> Could break here, or just return false.

Yes I could just bail here, but I am trying to report all possible errors. In
some cases it is not possible to proceed - loop index could not be identified.
In other cases I traverse the whole tree and report all errors.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:445: valid = false;
On 2010/11/22 23:33:52, kbr1 wrote:
> Why not just return false?

see comment above.

http://codereview.appspot.com/3225041/diff/2001/src/compiler/ValidateLimitati...
src/compiler/ValidateLimitations.cpp:454: valid = false;
On 2010/11/22 23:33:52, kbr1 wrote:
> Same here. It seems safer to skip the above work if the first checks failed.

ditto.

[dgkoch, 2010-11-23 21:06:28]

LGTM. will leave final call up to KBR

[kbr1, 2010-11-23 21:52:44]

LGTM. Fantastic work!

[Alok Priyadarshi, 2010-11-24 18:38:21]

On 2010/11/23 21:52:44, kbr1 wrote:
> LGTM. Fantastic work!

WebGL conformance tests and layout tests pass too. Submitting.