Cycles: Cleanup BSDF eval switch
Needs ReviewPublic
Actions

Authored by Thomas Dinges (dingto) on Apr 21 2015, 11:01 AM.

Details

Reviewers

Summary

Hi,
little cleanup of the bsdf eval switch. We have a lot of function calls here (functions are not inlined), which just return 0.0, so we can assign the value directly, like in the default case.

This could be simplified further, by removing the corresponding functions altogether. OSL would use the default eval_reflect and eval_transmit functions (macro in osl_closures.h), which default to 0.0 as well.

Quick performance test with branched_path_trace_blend yield a ~1% performance improvement. Main question here is, if that approach is good or makes code less self explaining.

Diff Detail

Event Timeline

Thomas Dinges (dingto) updated this revision to Diff 4040.Apr 21 2015, 11:01 AM

Thomas Dinges (dingto) retitled this revision from to Cycles: Cleanup BSDF eval switch.

Thomas Dinges (dingto) updated this object.

Thomas Dinges (dingto) added a reviewer: Sergey Sharybin (sergey).

Not totally happy with the BSDF-specific logic spreading outside of related files. Would be better to look into inlining the small functions instead i think (also worth checking if they're not being inlined by compiler already).

Second attempt, just get rid of the 0.0 reflect/transmit functions.

I think that is self explaining, but added a clear comment in the bsdf_eval() function. We also only have blur functions in BSDFs that actually support this, I think the same makes sense here.

@Brecht Van Lommel (brecht) Any opinion on this? :)

I prefer not to do this, I find having the logic for one BSDF in one file easier to understand and modify. I would expect these functions to be inlined by the compiler.

It kind of makes sense to keep case statements and have kernel_assert() in the default section to catch missing cases.

Also, did you do any bench marks?

Eeh, sorry for the spam. Had anyone did a comparison of assembly outputs? :) That's kinda really interesting to know what compilers are capable of.