Page MenuHome

Cycles: Declare constants at program scope on Metal
ClosedPublic

Authored by Michael Jones (michael_jones) on Nov 16 2021, 2:39 PM.

Details

Summary

MSL requires that constant address space literals be declared at program scope. This patch moves the blackbody_table_r/g/b and cie_colour_match constants into separate files so they can be declared at the appropriate scope.

Ref T92212

Diff Detail

Repository
rB Blender
Branch
arcpatch-D13241 (branched from master)
Build Status
Buildable 18737
Build 18737: arc lint + arc unit

Event Timeline

Michael Jones (michael_jones) requested review of this revision.Nov 16 2021, 2:39 PM
Michael Jones (michael_jones) created this revision.

Assuming it works for other platforms, I think it would be neater to just move the constants to program scope and avoid the conditional includes. I'm open to suggestions!

Brecht Van Lommel (brecht) requested changes to this revision.Nov 16 2021, 5:27 PM

I think we can indeed have these at program scope for all devices.

These were actually at program scope until recently, so should be fine. rB76de3ac4ce43: Cleanup: Remove data duplication from various lookup tables in Cycles.

These are small tables so I'm not particularly concerned about duplication.

This revision now requires changes to proceed.Nov 16 2021, 5:27 PM

Would MSL support these defined globally but as inline constexpr instead of static? That would also remove the duplication that occurs if these headers are used in multiple TUs.

Would MSL support these defined globally but as inline constexpr instead of static? That would also remove the duplication that occurs if these headers are used in multiple TUs.

Specifying inline is a no-op in MSL and gives a warning ("inline variables are a C++17 extension"), however MSL does require that any program scope variable must reside in constant address space.

We should probably hide the qualifiers behind a macro which can be defined on a per-platform basis.

These were actually at program scope until recently, so should be fine. rB76de3ac4ce43: Cleanup: Remove data duplication from various lookup tables in Cycles.

The previous constants' locations (as in rB76de3ac4ce43: Cleanup: Remove data duplication from various lookup tables in Cycles) would in fact be at class scope on Metal due to the context class. The simplest solution to this would be to include the constants in a monolithic block before including Metal's "context_begin.h", at the loss of some locality of reference (although I'm happy to consider other approaches if they seem better).

I think it would be fine to move all the tables into a single file that we can include ahead of context_begin.h.

  • Move all constant tables into tables.h, included at program scope
  • Disable clang formatting which caused unwanted include reordering

Looks fine, but I think we could try the suggestion from @Jesse Yurkovich (deadpin)?

Maybe rename ccl_static_constant to ccl_inline_constant, and then define that as inline constexpr for the CPU device, and leave it unchanged for GPU devices?

I'm not sure if it's better to use /* clang-format off */ or just add a line break after #include "kernel/tables.h".
(Just an observation).

  • Rename ccl_static_constant to ccl_inline_constant and define as inline constexpr on CPU

I need to add one fix for OptiX kernel building. I'll commit this patch myself with that fix included.

This revision is now accepted and ready to land.Nov 18 2021, 2:57 PM