Unroll for-loop if sampler array uses loop index as its index.

Unroll for-loop if sampler array uses loop index as its index.

If inside a for-loop, sampler array index is the loop index, Mac cg compiler will crash.  This CL unroll the loop in such situation.  The behavior is:
1) If the for-loop index is a float, we reject the shader.
2) If it is an integer, we unroll the for-loop.

Things that should be done in the future are:
1) Add line number macros.
2) Add a limit to unroll iteration count.

anglebug=94
Committed: http://code.google.com/p/angleproject/source/detail?r=606

[Zhenyao Mo, 2011-03-30 22:46:19]

Please review.

[Zhenyao Mo, 2011-04-01 17:14:09]

Ping.

[Alok Priyadarshi, 2011-04-01 17:17:43]

Overall looks fine.

A couple of concerns/suggestions:
1. Unrolling should be controlled by a flag that can be controlled by clients.
Always doing it on Mac does not seem right. Chrome may always want to unroll but
Mozilla/Apple may not.
2. If you add it as an option, it would be better to keep it separate from
validate-limitations and output-glsl. This could be a separate pass (similar to
ValidateLimitations) which modifies the AST.
3. You may have already considered this but did not look very closely. How do
you handle cases if the sampler index is an expression which uses the loop
index.

[zmo, 2011-04-01 17:22:59]

Thanks for reviewing this.

On 2011/04/01 17:17:43, Alok Priyadarshi wrote:
> Overall looks fine.
> 
> A couple of concerns/suggestions:
> 1. Unrolling should be controlled by a flag that can be controlled by clients.
> Always doing it on Mac does not seem right. Chrome may always want to unroll
but
> Mozilla/Apple may not.

My feeling is we don't need an option.  It's either unroll or crash, and crash
is not an option.

This unroll will be removed once Apple fixes their bug, so it's a temporary
thing.

> 2. If you add it as an option, it would be better to keep it separate from
> validate-limitations and output-glsl. This could be a separate pass (similar
to
> ValidateLimitations) which modifies the AST.
> 3. You may have already considered this but did not look very closely. How do
> you handle cases if the sampler index is an expression which uses the loop
> index.

I do the checking and replacement in visitSymbol(), so even if the index is an
expression, it's still handled.

[Alok Priyadarshi, 2011-04-01 17:29:29]

> 
> My feeling is we don't need an option.  It's either unroll or crash, and crash
> is not an option.
> 

I think we should have an option for anything that is non-standard (i.e. not in
the spec). It is also nice for debugging purposes. I will leave it to Daniel and
Ken to make the call.

[zmo, 2011-04-01 17:33:44]

On 2011/04/01 17:22:59, zmo wrote:
> Thanks for reviewing this.
> 
> On 2011/04/01 17:17:43, Alok Priyadarshi wrote:
> > Overall looks fine.
> > 
> > A couple of concerns/suggestions:
> > 1. Unrolling should be controlled by a flag that can be controlled by
clients.
> > Always doing it on Mac does not seem right. Chrome may always want to unroll
> but
> > Mozilla/Apple may not.
> 
> My feeling is we don't need an option.  It's either unroll or crash, and crash
> is not an option.
> 
> This unroll will be removed once Apple fixes their bug, so it's a temporary
> thing.
> 
> > 2. If you add it as an option, it would be better to keep it separate from
> > validate-limitations and output-glsl. This could be a separate pass (similar
> to
> > ValidateLimitations) which modifies the AST.
> > 3. You may have already considered this but did not look very closely. How
do
> > you handle cases if the sampler index is an expression which uses the loop
> > index.
> 
> I do the checking and replacement in visitSymbol(), so even if the index is an
> expression, it's still handled.

Confirmed: the current unroll works fine with index in an expression, and in
nested loops.

[zmo, 2011-04-01 17:43:18]

OK, let me explain my reasoning before Judge Ken makes the final call:

1) If we make it an option, so we implement it, and various users of ANGLE will
have to change their code to make it effective.  And later if Apple fixes their
bug, we remove this unrolling, and ANGLE users again have to change their code
back.

As an alternative, we do it silently (although I already emailed ANGLE users
Apple and Firefox about this fix and they had no objections), and later we
removed it silently, and our users just need to keep rolling ANGLE without
changing their code.

2) It's really a very very corner case; we don't unroll in general, but only
when Sampler arrays in a loop, and array index is also the loop index.  So even
naming the option correctly is challenging.

On 2011/04/01 17:29:29, Alok Priyadarshi wrote:
> I think we should have an option for anything that is non-standard (i.e. not
in
> the spec). It is also nice for debugging purposes. I will leave it to Daniel
and
> Ken to make the call.

[kbr1, 2011-04-01 20:30:32]

It would be better if this patch were less intrusive and implemented as a
separate pass over the AST, since we plan to remove it at some point. Right now
many places in the code are touched, and it's going to be difficult to undo
those changes in the future.

[zmo, 2011-04-01 23:04:07]

Per discussion with kbr off-line, although keeping it as a separate pass and
unrolling the loop in AST is a neater solution, it requires much more efforts. 
Since this is only a temporary hack, the current solution minimizes the code
change and the possibility of introducing new bugs.

Fixed a few issues kbr pointed out during our off-line discussion.  Please
review again.

[kbr1, 2011-04-04 19:14:37]

Thanks for doing the cleanups and adding a comment pointing back to this CL.

LGTM