Bug 48375

The test is failing because the vertex shader fails to compile---we run out of registers and haven't implemented VS spilling yet.

The loop of interest is:
void main() {
    int i;
    for (i = 0; i < gl_MaxLights; i++) {
        ...do various function calls, passing i as an 'in' parameter...
    }
}

Previously, we would unroll this right away, ignoring the function calls completely.  They would later get inlined.  This resulted in no loop.

Now, we delay unrolling until after function inlining and other optimizations.  It looks like something is rewriting the code enough that loop analysis gets confused, and fails to unroll the loop.  This then triggers an issue in the i965/vs backend, where our register liveness analysis within loops is basically "uh, it must be alive".  So we run out of registers.  And then we don't support spilling.  So it doesn't compile.  And then nothing works.

I see several solutions to this bug, all of which sound difficult:
1. Make loop analysis smarter about function calls.

   In this case, since the induction variable 'i' is local to main(), and it's
   only ever passed as an 'in' parameter, it isn't possible for any of the
   function calls to alter it.  So our analysis is safe here.

2. Make loop analysis/unrolling smarter in general.

   It ought to be able to unroll the loop even after function inlining.
   We already know that the unroller is hackish, and had planned to replace it
   at some point.  It's just been ZBB'd repeatedly.

3. Make i965's liveness interval computations smarter in the presence of loops.

   The backend really ought to be able to figure this out.

4. Implement register spilling in the VS backend.

   This should be simple enough and is worth doing.  It would get the test
   passing again.  But there would still be a large code quality regression;
   spilling is bad and should be avoided at almost any cost.

It fails on Sandybridge, Ivybridge and Ironlake with mesa master branch.It doesn't happen on mesa 8.0 branch.

It also happens on Pineview with mase master branch.

It also happens on mesa 9.0 branch(commit:6886da783ac2fc549b4ffc1f4)

Unfortunately, it's too late in the release process to fix this for 9.0.  Perhaps it can be fixed in a subsequent 9.0.x release.

It doesn't make sense to revert the patch because it fixes a critical bug where we generated completely incorrect code for certain shaders.  Plus, reverting it would regress other Piglit tests...

Changes from crash to fail with my VS live intervals code.  Having read through about half the dump, the register allocation is looking reasonable so far and there's no spilling, so I'm wondering if there may be other issues with the change from unrolled to looping.

One significant change is the static array indexing with unrolled.  That code also looks pretty good to me, other than a small note that uniform_vector_size[] is 4 for the scalar components of the structure because of the hilariously broken last_swiz test in setup_builtin_uniforms().  I don't think that's hurting us, though.

Now that the state of this testcase has changed significantly as of:

commit 20ebebac5153affcbd44350332678a2fb04d4c96
Author: Eric Anholt <eric@anholt.net>
Date:   Wed Oct 3 13:44:12 2012 -0700

    i965/vs: Improve live interval calculation.

I've replaced this bug report with bug #56104 because I think it's just revealing an existing underlying issue (variable-indexed array access on builtin uniforms?  What are the chances there's a bug there?) and all the spilling discussion here will just distract us.

It still fails on commit 6c281749696da417a88c3d1ee460f642f92a4cee).

Yes, I said it was still failing, and I'm moving the remaining, non-regression bug handling to a fresh bug report so that we don't have to wade through the fixed issue when looking at hte bug report for this testcase.

Fixed on master branch by commit 7e28d6c1ab101894c3a3c90bef929c34e9da6148.
commit 7e28d6c1ab101894c3a3c90bef929c34e9da6148
Author:     Eric Anholt <eric@anholt.net>
AuthorDate: Wed Nov 21 14:30:55 2012 -0800
Commit:     Eric Anholt <eric@anholt.net>
CommitDate: Fri Dec 28 10:53:50 2012 -0800

    i965: Consistently use nr_pull_params instead of NumParameters.

    NumParameters used to be an upper bound on the number of vec4s to be
    uploaded, which was basically safe (unless your buffer was bound near
    the top of address space *and* you array indexed outside the buffer, in
    which case I think you might GPU hang).  As I migrate the driver away
    from ParameterValues[], this is no longer true.