Review: improve adjust_varying

adjust_varying is the function that doctors whether a variable is uniform or varying based on what is about to be assigned to it.  But if a uniform is being assigned from within a conditional, it needs to be a varying assignment, since it should only happen on some shading points.

A few days back, Chris Kulla observed that adjust_varying was making this decision based on all_points_on() as a proxy for whether we were in a conditional or not... BUT... with the "sparse" batches we are shading for secondary rays, all points are not turned on, even when we aren't inside a conditional!  So in theory, we weren't taking nearly as much advantage of uniforms as we should be on secondary rays (and therefore not getting full performance from batching).

The attached patch addresses this by directly tracking when we are in nested conditionals, and using that as the basis for the decision.  Result: at least 25% rendering speedup on production assets!

There's also a little bit of additional infrastructure to ensure we handle orig_runflags carefully, and optionally (by compile-time #define) gather stats about adjust_varying to verify that it works as expected.

[ckulla, 2010-02-06 02:48:40]

Seems like a good change. A few questions about the implementation: (maybe just
needs more comments)

http://codereview.appspot.com/204046/diff/1/6
File src/liboslexec/exec.cpp (right):

http://codereview.appspot.com/204046/diff/1/6#newcode386
src/liboslexec/exec.cpp:386: Runflag *orig = context()->m_original_runflags;
Why is this save/restore needed?

http://codereview.appspot.com/204046/diff/1/6#newcode534
src/liboslexec/exec.cpp:534: run (beginop, endop);
Why is m_conditional_level not set for this case?

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

http://codereview.appspot.com/204046/diff/1/4#newcode110
src/liboslexec/opcontrol.cpp:110: exec->enter_conditional ();
Why is this needed? Can't it be handled inside push/pop runflags?

http://codereview.appspot.com/204046/diff/1/2
File src/liboslexec/oslexec_pvt.h (right):

http://codereview.appspot.com/204046/diff/1/2#newcode67
src/liboslexec/oslexec_pvt.h:67: #define NO_DEBUG_ADJUST_VARYING
I would prefer having:

// #define DEBUG_ADJUST_VARYING

[solomon.boulos, 2010-02-06 04:04:07]

http://codereview.appspot.com/204046/diff/1/6
File src/liboslexec/exec.cpp (right):

http://codereview.appspot.com/204046/diff/1/6#newcode704
src/liboslexec/exec.cpp:704: varying_assignment |= ! running_top_level();
running_top_level() suggests that you can't be inside any conditional (as does
the comment above). Of course the real logic is that you can't have "diverged".
Why not express it in terms of has_not_diverged()?

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

http://codereview.appspot.com/204046/diff/1/4#newcode172
src/liboslexec/opcontrol.cpp:172: // From here on, varying condition or
potential
Like the if clause above, it seems that we should be checking to see whether or
not the condition is actually identical for all values. That would mean that it
isn't really "varying", we just didn't notice... The same thing holds for
dowhile below.

[lg_imageworks.com, 2010-02-06 06:43:56]

On Feb 5, 2010, at 6:48 PM, <ckulla@gmail.com> <ckulla@gmail.com> wrote:

> http://codereview.appspot.com/204046/diff/1/6#newcode386
> src/liboslexec/exec.cpp:386: Runflag *orig =
> context()->m_original_runflags;
> Why is this save/restore needed?

I think it's not.  I added it at one point that I was tracking down a bug and
never removed it (figuring that it couldn't hurt, and might protect us from
subtle bugs some day).  I can remove it.


> http://codereview.appspot.com/204046/diff/1/6#newcode534
> src/liboslexec/exec.cpp:534: run (beginop, endop);
> Why is m_conditional_level not set for this case?

Because that branch is for init ops.  Um...  In retrospect, I guess it
can/should be set for both clauses.  Let me try it and if nothing breaks I'll do
it.

> http://codereview.appspot.com/204046/diff/1/4#newcode110
> src/liboslexec/opcontrol.cpp:110: exec->enter_conditional ();
> Why is this needed? Can't it be handled inside push/pop runflags?

We push/pop runflags for other reasons.  Such as in the main run() function.  In
that case, and possibly other future ones, pushing runflags doesn't necessarily
imply that some points have been turned off, only that you want to save the
existing runflags in case something later mucks with them.  So I do
enter/exit_conditional specifically in the places where we reduce to subsets of
runflags, which is where it's needed.


> http://codereview.appspot.com/204046/diff/1/2#newcode67
> src/liboslexec/oslexec_pvt.h:67: #define NO_DEBUG_ADJUST_VARYING
> I would prefer having:
> 
> // #define DEBUG_ADJUST_VARYING

Sure, why not.


--
Larry Gritz
lg@imageworks.com

[lg_imageworks.com, 2010-02-06 06:58:24]

On Feb 5, 2010, at 8:04 PM, <solomon.boulos@gmail.com>
<solomon.boulos@gmail.com> wrote:

> http://codereview.appspot.com/204046/diff/1/6#newcode704
> src/liboslexec/exec.cpp:704: varying_assignment |= !
> running_top_level();
> running_top_level() suggests that you can't be inside any conditional
> (as does the comment above). Of course the real logic is that you can't
> have "diverged". Why not express it in terms of has_not_diverged()?

Yeah, I suppose that's more clear.  Chris?  Anybody else?  Opinions?


> http://codereview.appspot.com/204046/diff/1/4
> File src/liboslexec/opcontrol.cpp (right):
> 
> http://codereview.appspot.com/204046/diff/1/4#newcode172
> src/liboslexec/opcontrol.cpp:172: // From here on, varying condition or
> potential
> Like the if clause above, it seems that we should be checking to see
> whether or not the condition is actually identical for all values. That
> would mean that it isn't really "varying", we just didn't notice... The
> same thing holds for dowhile below.

I'll put in a FIXME for now. 

--
Larry Gritz
lg@imageworks.com

[boulos_cs.stanford.edu, 2010-02-06 07:03:43]

On Feb 5, 2010, at 10:58 PM, Larry Gritz wrote:

> On Feb 5, 2010, at 8:04 PM, <solomon.boulos@gmail.com>
<solomon.boulos@gmail.com> wrote:
> 
>> http://codereview.appspot.com/204046/diff/1/6#newcode704
>> src/liboslexec/exec.cpp:704: varying_assignment |= !
>> running_top_level();
>> running_top_level() suggests that you can't be inside any conditional
>> (as does the comment above). Of course the real logic is that you can't
>> have "diverged". Why not express it in terms of has_not_diverged()?
> 
> Yeah, I suppose that's more clear.  Chris?  Anybody else?  Opinions?

Chris and I agreed about this on chat (if that's any consolation). I'm fine with
some other name, I just don't like running_top_level() ;).

I forgot to also second Chris's suggestion that the enter/exit stuff be moved
into push/pop_runflags() (which despite sounding like a general utility function
is _only_ used when the runflags "changes" -- modulo the note below).

> 
> 
>> http://codereview.appspot.com/204046/diff/1/4
>> File src/liboslexec/opcontrol.cpp (right):
>> 
>> http://codereview.appspot.com/204046/diff/1/4#newcode172
>> src/liboslexec/opcontrol.cpp:172: // From here on, varying condition or
>> potential
>> Like the if clause above, it seems that we should be checking to see
>> whether or not the condition is actually identical for all values. That
>> would mean that it isn't really "varying", we just didn't notice... The
>> same thing holds for dowhile below.
> 
> I'll put in a FIXME for now. 

Sounds good. Just more fuel for optimization (though I doubt people are
currently using this -- except doesn't the main() get a do/while inserted into
it by default? I guess that do/while is statically uniform though...)

> 
> --
> Larry Gritz
> lg@imageworks.com
> 
> 
> 
> 

[lg_imageworks.com, 2010-02-06 07:17:49]

On Feb 5, 2010, at 11:03 PM, Solomon Boulos wrote:
> 
> I forgot to also second Chris's suggestion that the enter/exit stuff be moved
into push/pop_runflags() (which despite sounding like a general utility function
is _only_ used when the runflags "changes" -- modulo the note below).
> 

That's not actually true.  We push when we first run the shader, and that
doesn't change the runflags.  And we need to, so that exit() will work
correctly.

I suppose we could have an optional argument to push_runflags that says whether
we expect the runflags to actually change, and modify the 'diverged' flag only
in that case.


--
Larry Gritz
lg@imageworks.com

[larrygritz, 2010-02-07 22:10:29]

Addressed some comments by Chris and Solomon

[larrygritz, 2010-02-07 22:12:11]

One more update, remove dead code

[larrygritz, 2010-02-07 22:13:15]

On 2010/02/07 22:12:11, larrygritz wrote:
> One more update, remove dead code

OK, here's what I'm proposing to check in now.  Addresses most of Chris' and
Solomon's comments.

One outstanding issue is whether to fold the conditional entry/exit into
push/pop runflags.  I'll take care of this when I revamp runflags, which is
next.

[ckulla, 2010-02-08 02:18:33]

LGTM