Review: compute derivatives only when needed

At long last, this patch switches from the old (intended to be temporary) behavior -- computing derivatives on all non-constant, non-int, non-closure symbols -- to correctly marking ONLY those symbols that truly need derivatives as such.  This involves the following steps:

0. oslc already marked which arguments to each op need derivatives.  For example, texture(name,s,t,...) takes derivatives of s and t.  But now when we load the shader, we correctly read that from the oso file and store it with each op.

1. For each shader layer, after runtime optimization but before temporary coalescing, we analyze the full variable dependencies and therefore mark all dependencies of symbols whose derivatives are taken as needing to have derivatives.

2. We proceed from LAST layer to FIRST (i.e. from downstream to upstream), and as we do the deriv dependency analysis on each layer, we also mark any upstream output parameters of earlier layers that are connected to our layer's input parameters that need derivatives.  In this way, derivative-need propagates through the shader network.

3. When we coalesce temporaries, we are careful not to coalesce a variable that needs derivs with one that doesn't!

4. Once this was done, I discovered that spline and texture were both broken when no derivatives were needed.  :-)  So those are fixed now.

5. Also some other minor refactoring, including moving the necessary dependency analysis from oslc into the runtime library.

Results: in our favorite production benchmark frame, after these changes only 7% of symbols are marked as needing derivatives.  This greatly reduces memory use, since the vast majority of symbols now only need to store their values, not also two differentials.  It also speeds up execution of the shader, since there's less math when you don't compute the derivatives.  But, alas, our production frames only speed up by 6-7%, mainly because the deriv math (or, indeed raw shader interpretation) is just not the big bottleneck in our renderer at this point.  (We're working on that on a second front, mostly not on the OSL side of the fence.)

[ckulla, 2010-04-05 17:58:24]

LGTM overall, but it looks to me like there are a few missing cases in the
texture op.

http://codereview.appspot.com/888041/diff/1/4
File src/liboslexec/optexture.cpp (right):

http://codereview.appspot.com/888041/diff/1/4#newcode199
src/liboslexec/optexture.cpp:199: if (Result.has_derivs() && S.has_derivs() &&
T.has_derivs()) {
What about Alpha->has_derivs() ?

http://codereview.appspot.com/888041/diff/1/4#newcode256
src/liboslexec/optexture.cpp:256: if (Result.has_derivs() && options.dresultds)
{
What about Result.has_derivs() vs. Alpha->has_derivs() ?

[lg_imageworks.com, 2010-04-05 18:35:59]

Good point, Chris.  I'll submit an updated patch in a moment that I think
addresses these cases.


On Apr 5, 2010, at 10:58 AM, <ckulla@gmail.com> <ckulla@gmail.com> wrote:

> LGTM overall, but it looks to me like there are a few missing cases in
> the texture op.
> 
> 
> http://codereview.appspot.com/888041/diff/1/4
> File src/liboslexec/optexture.cpp (right):
> 
> http://codereview.appspot.com/888041/diff/1/4#newcode199
> src/liboslexec/optexture.cpp:199: if (Result.has_derivs() &&
> S.has_derivs() && T.has_derivs()) {
> What about Alpha->has_derivs() ?
> 
> http://codereview.appspot.com/888041/diff/1/4#newcode256
> src/liboslexec/optexture.cpp:256: if (Result.has_derivs() &&
> options.dresultds) {
> What about Result.has_derivs() vs. Alpha->has_derivs() ?
> 
> http://codereview.appspot.com/888041/show
> 

--
Larry Gritz
lg@imageworks.com

[larrygritz, 2010-04-05 18:38:18]

Address Chris' comments about result vs alpha derivs

[ckulla, 2010-04-05 18:41:50]

LGTM

One minor comment, but it doesn't affect correctness.

http://codereview.appspot.com/888041/diff/12001/13003
File src/liboslexec/optexture.cpp (right):

http://codereview.appspot.com/888041/diff/12001/13003#newcode259
src/liboslexec/optexture.cpp:259: if ((Result.has_derivs() || (Alpha &&
Alpha->has_derivs())) &&
I think this test can stay as just:

if (options.dresultds) { ...

because we leave it at NULL if we didn't need derivs.

[lg_imageworks.com, 2010-04-05 18:55:41]

OK.  Thanks for the tip.


On Apr 5, 2010, at 11:41 AM, <ckulla@gmail.com> <ckulla@gmail.com> wrote:

> LGTM
> 
> One minor comment, but it doesn't affect correctness.
> 
> 
> http://codereview.appspot.com/888041/diff/12001/13003
> File src/liboslexec/optexture.cpp (right):
> 
> http://codereview.appspot.com/888041/diff/12001/13003#newcode259
> src/liboslexec/optexture.cpp:259: if ((Result.has_derivs() || (Alpha &&
> Alpha->has_derivs())) &&
> I think this test can stay as just:
> 
> if (options.dresultds) { ...
> 
> because we leave it at NULL if we didn't need derivs.
> 
> http://codereview.appspot.com/888041/show
> 

--
Larry Gritz
lg@imageworks.com

[larrygritz, 2010-04-05 21:39:33]

One more comment -- even though this only speeds up our production frames by
~6%, on testshade torture tests (lots and lots of math ops, no texture, no
lighting integration) it speeds up by 50%, which is about what I expected when
you cut out the deriv math on most of the symbols.  So this works great and as
intended, it's just that executing the OSL ops themselves is not the main
bottleneck in our renderer at this point.