Cycles: Add Debug build option that enables floating point exceptions
Needs RevisionPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Jun 6 2016, 10:46 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Thomas Dinges (dingto)
Sergey Sharybin (sergey)

Summary

This option is mainly useful for tracking down NaNs and other numerical problems
by throwing a floating point exception right at the invalid operation.
The BVH traversal generates a lot of false positives, though, so it's disabled in there.

I didn't test the C++11/MSVC options yet, so if someone could test building on Windows, that'd be great.

Diff Detail

Repository: rB Blender
Branch: master

Event Timeline

Lukas Stockner (lukasstockner97) updated this revision to Diff 6865.Jun 6 2016, 10:46 PM

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Add Debug build option that enables floating point exceptions.

Lukas Stockner (lukasstockner97) added reviewers: Sergey Sharybin (sergey), Thomas Dinges (dingto), Brecht Van Lommel (brecht).

Lukas Stockner (lukasstockner97) updated this object.

Not sure this should be a build option, for Blender it can be enabled at run-time (Cycles could even use Blender's --debug-fpe argument, and the stand-alone version can take its own arg).

Also hows does this interact with Blender's --debug-fpe? from quick check seems it may overwrite those settings.

Is there any reason why using Blender's ` fpe debug option isn't sufficient?

@Campbell Barton (campbellbarton), i wouldn't want any extra debug stuff happening in kernel, those things does not come for free. And you _do_ need to disable FPE in certain places of kernel.

Yes, that was my reasoning as well - a build option has zero runtime cost if it's disabled, but adding an if() into a function that's called billions of times during rendering might have a noticable performance impact...

Regarding interfering with --debug-fpe: Yes, that's likely. I guess the easiest fix for that is to store the previous state in the constructor and restore it afterwards, that'd also solve a possible problem with nesting scoped_fpe: If you disable FPE in two nested functions, it will currently be enabled after the inner function returns.

Ah, if its going to interfere with regular operations, of course it's better as an ifdef.

Even so, it would be good to give some brief info on how this is different --debug-fpe (limits scope).

And don't think its so important to restore previous debug handlers, since its not expected to leave this on in regular builds, could just note that it clobbers the handler set by --debug-fpe arg.

Generally fine, can extend CMake option comment/description to make Capbell happy :)

Some minor code issue tho.

intern/cycles/util/util_system.cpp
306	This is a very wrong way to initialize members. We should get rid of tendency of using underscore for function arguments and use underscore for private members instead. That being said, this is a correct code as well: scoped_fpe::scoped_fpe(bool enable) : enable(enable)
intern/cycles/util/util_system.h
46	For single argument constructors use `explicit` qualifier. I also think we should use enum value instead of a bool to have it more readable. `fpe(false)`/`fpe(true)` does not read well actually.