Reviewers: ,

Description:
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.)



Please review this at http://codereview.appspot.com/888041/show

Affected files:
src/include/osl_pvt.h
src/liboslexec/instance.cpp
src/liboslexec/loadshader.cpp
src/liboslexec/opspline.cpp
src/liboslexec/optexture.cpp
src/liboslexec/oslexec_pvt.h
src/liboslexec/runtimeoptimize.cpp
src/liboslexec/shadingsys.cpp

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

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, <cku...@...> <cku...@...> 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
l...@...