Page MenuHome
Differential D2089

Cycles: Add helper functions for constant folding boilerplate.
ClosedPublic
Actions

Authored by Brecht Van Lommel (brecht) on Jul 7 2016, 1:09 PM.

Details

Reviewers
Alexander Gavrilov (angavrilov)
Group Reviewers
Cycles
Commits
rC0fcc7cbc6b81: add ConstantFolder class for constant folding boilerplate.
rBS5234e9ddd3ec: Cycles: add ConstantFolder class for constant folding boilerplate.
rB5234e9ddd3ec: Cycles: add ConstantFolder class for constant folding boilerplate.
Summary

All constant folding operations involve one of these two statement
patterns. Adding tiny helper functions allows reducing them to
one-statement tail calls, thus reducing line count and chance
of error re when to return true.

For the RGB Mix node an additional helper can be extracted as a
method, to keep the necessary parameter count as 4 instead of 7.

Diff Detail

Repository
rB Blender

Event Timeline

Alexander Gavrilov (angavrilov) updated this revision to Diff 7020.Jul 7 2016, 1:09 PM
Alexander Gavrilov (angavrilov) retitled this revision from to Cycles: Add helper functions for constant folding boilerplate..
Alexander Gavrilov (angavrilov) added a reviewer: Cycles.
Alexander Gavrilov (angavrilov) updated this object.
Alexander Gavrilov (angavrilov) updated this revision to Diff 7026.Jul 8 2016, 11:03 AM

Added {} back.

Sergey Sharybin (sergey) added subscribers: Thomas Dinges (dingto), Brecht Van Lommel (brecht), Sergey Sharybin (sergey).Jul 12 2016, 10:29 AM

Like how try_fold_as_noop() simplified MixNode. Other changes also seems fine to me but since they kind of depend on particular interpretation (which is with this patch: we just delegate optimization to fold_as_<value>, don't want to care about anything ourselves after that) would be nice to hear from @Brecht Van Lommel (brecht), @Thomas Dinges (dingto) and others.

intern/cycles/render/nodes.cpp
35

Arguably, should it actually be a function or a method of Graph / ShaderNode / ShaderInput?

For now functions will work fine, but making it a method can save us passing some arguments perhaps. Just thinking we can have:

bool ShaderInput::set_optimized_value(float3 value);

And things like that. We might add extra sanity checks there like assert(there_is_no_connected_links_to_this_input);.

Just an idea which we can do later if this new approach is accepted by everyone.

Alexander Gavrilov (angavrilov) added inline comments.Jul 12 2016, 10:40 AM
intern/cycles/render/nodes.cpp
35

We might add extra sanity checks there like assert(there_is_no_connected_links_to_this_input);.

Actually, links are removed by the main constant folding function if constant_fold returned true, so at this point there definitely are links. The main function also copies the value to other sibling users from this one ShaderInput that was directly set here. This does feel a bit weird so maybe now there's fold_as_value, it should do all that stuff instead?..

Brecht Van Lommel (brecht) added a comment.Jul 14 2016, 10:58 PM

I think this is fine. Though would prefer to not to do clamping in these utility functions, would rather see that being done in the SVM function to ensure things do not go out of sync.

If we get a bunch of such utility functions, perhaps a good solution is to introduce a ConstantFolder class. ShaderNode, ShaderGraph, etc. are pretty big already. Something like:

bool MathNode::constant_fold(ConstantFolder& folder)
{
    if(folder.all_inputs_constant()) {
        return folder.set_constant_value(svm_math(type, value1, value2));
    }

    ...
}
Alexander Gavrilov (angavrilov) added a comment.EditedJul 15 2016, 10:45 AM
In D2089#48524, @Brecht Van Lommel (brecht) wrote:

Though would prefer to not to do clamping in these utility functions, would rather see that being done in the SVM function to ensure things do not go out of syn

The thing is, SVM does clamping as a separate node operation, so there is no 'in the SVM function' there either. And that operation is generated by compile methods in nodes.cpp itself.

A new utility class for passing the constant folding related parameters around may be a good idea though. set_constant_value is rather long winded though, maybe folder.make_constant, folder.make_noop etc would be sufficient?

Alexander Gavrilov (angavrilov) updated this revision to Diff 7057.Jul 15 2016, 12:55 PM

Implemented Brecht suggestion about a ConstantFolder helper class to contain folding parameters and functions.

Alexander Gavrilov (angavrilov) updated this revision to Diff 7059.Jul 15 2016, 3:59 PM

Add one more ConstantFolder method for Closure folding and an Add Closure handler.

Brecht Van Lommel (brecht) commandeered this revision.Jul 16 2016, 8:55 PM
Brecht Van Lommel (brecht) added a reviewer: Alexander Gavrilov (angavrilov).

I like it, but I made some further changes:

  • Don't return bool from constant_fold anymore if it's not used anyway.
  • Rename "make no-op" and "substitute" to "bypass", I think that better describes what these functions do.
  • Don't use process_fold(), seems like an unnecessary indirection.
  • Pass ConstantFolder by reference since it can't be NULL (we don't do this consistently but think it's good practice anyway).
  • Move all_inputs_constant() into ConstantFolder.
  • Some code style changes to follow conventions.
Brecht Van Lommel (brecht) updated this revision to Diff 7064.Jul 16 2016, 8:56 PM

Further code refactoring, no functional changes.

Sergey Sharybin (sergey) added inline comments.Jul 16 2016, 9:03 PM
intern/cycles/render/constant_fold.h
27

In other areas we put const in the front.

intern/cycles/render/nodes.cpp
1583

Is it a nifty way to save one line of code with a searate return?

Feel weird about reutning a void value. And either having memory glitch, or some compilers might generate warning?

intern/cycles/render/nodes.h
292

<Controversiality>
For own projects found it more readable/reliable to use const references only (so you don't accidentally assign value and so) and for non-const use pointers.

What do you guys think of using such approach in Cycles as well?
</Controversiality>

Thomas Dinges (dingto) added a comment.Jul 16 2016, 9:04 PM

Some comments. Otherwise looks good to me.

intern/cycles/render/constant_fold.cpp
3

Same as above.

25

Too many includes, don't think we need half of them.

intern/cycles/render/constant_fold.h
3

Picky, should be 2011-2016 as it's new code.

Alexander Gavrilov (angavrilov) added inline comments.Jul 16 2016, 9:12 PM
intern/cycles/render/constant_fold.h
27

It's just weirdness of the C/C++ syntax:

  • const Foo *bar is a mutable field bar that contains a pointer to immutable Foo.
  • Foo *const bar is an immutable field bar that contains a pointer to mutable Foo.
Brecht Van Lommel (brecht) added inline comments.Jul 16 2016, 9:58 PM
intern/cycles/render/nodes.cpp
1583

It's valid C++, but since it's causing confusion I'll change it.

intern/cycles/render/nodes.h
292

I don't really see the point of using using references and pointers to distinguish between const and non-const, I rather use it to distinguish between nullable and non-nullable. That way you can train yourself to remember to check for NULL whenever you use ->.

But we are not doing that consistently in Cycles at all, so there's not much value in that at the moment and I don't care much which convention we use.

Brecht Van Lommel (brecht) updated this revision to Diff 7067.Jul 16 2016, 9:58 PM

Address comments.

Sergey Sharybin (sergey) added inline comments.Jul 17 2016, 3:44 PM
intern/cycles/render/constant_fold.h
27

Oh, well. Guess we'll just live with that and accept C++ weirdeness. Not as if we've got options here.

intern/cycles/render/nodes.cpp
1583

Interesting. Coz i remember having issues with such (or similar but more tricky?) case.

If it's valid, i'm not fussed at all.

But code after changes i like more, since it leaves no room to confusion.

intern/cycles/render/nodes.h
292

The point is to avoid possible override of external data. It's quite often in code you operate on arguments directly (like, ensuring they're within a range) instead of using temp variables for that. And it's not rare this causes issues, especially when something is passed by reference.

The point of NULL-able is valid tho. So we need two types of references! :)

But we are not doing that consistently in Cycles at all, so there's not much value in that at the moment and I don't care much which convention we use.

That's what makes me sad. Consistency level is not great :(

But let's leave this for another discussion, outside of this patch.

Alexander Gavrilov (angavrilov) edited edge metadata.Jul 18 2016, 5:55 PM

So what's holding this up now? :)

intern/cycles/render/nodes.h
292

The point is to avoid possible override of external data.

Actually the main point about non-const references in C++ is that at the *call site* you can't immediately see if the parameter can be modified by the called function or not. For comparison, when passing by reference in C# the ref keyword is required both in parameter declaration and in the actuall call so it's obvious.

Here however it's not much of a problem, especially because even without const all fields of the class are constant.

Closed by commit rB5234e9ddd3ec: Cycles: add ConstantFolder class for constant folding boilerplate. (authored by Alexander Gavrilov (angavrilov), committed by Brecht Van Lommel (brecht)). · Explain WhyJul 18 2016, 10:54 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
intern/
cycles/
render/
2 lines
59 lines
122 lines
8 lines
36 lines
43 lines
338 lines

Diff 7087

View Options

intern/cycles/render/CMakeLists.txt

Loading...
View Options

intern/cycles/render/constant_fold.h

Loading...
View Options

intern/cycles/render/constant_fold.cpp

Loading...
View Options

intern/cycles/render/graph.h

Loading...
View Options

intern/cycles/render/graph.cpp

Loading...
View Options

intern/cycles/render/nodes.h

Loading...
View Options

intern/cycles/render/nodes.cpp

Loading...