Cycles: Integrator array float3 packing considerations for MSL
AbandonedPublic
Actions

Authored by Michael Jones (michael_jones) on Nov 16 2021, 12:59 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
William Leeson (leesonw)

Maniphest Tasks

T92212: Cycles Metal device

Summary

This patch allows float3 integrator arrays to be declared using an explicit 12 byte packed type, if necessary. On Metal, we declare #define integrator_float3 packed_float3 since float3 is a padded 16 byte type in MSL.

Ref T92212

Diff Detail

Repository

rB Blender

Branch

master

Build Status

Buildable 18684
Build 18684: arc lint + arc unit

Event Timeline

Michael Jones (michael_jones) requested review of this revision.Nov 16 2021, 12:59 PM

Michael Jones (michael_jones) created this revision.

Harbormaster completed remote builds in B18684: Diff 44797.Nov 16 2021, 1:00 PM

I'd prefer to have a packed_float3 defined for all devices, that implicitly casts to/from float3. It's not really specific to the integrator state, we also want to be able to use this in other data structures.

Do you think D13243: Cycles: add packed_float3 type for storage instead of this would work for Metal?

In D13236#349455, @Brecht Van Lommel (brecht) wrote:

Do you think D13243: Cycles: add packed_float3 type for storage instead of this would work for Metal?

Yes, I prefer the explicitness of D13243: Cycles: add packed_float3 type for storage and think it paves the way for further optimisations :)

The specific issue that this patch is addressing relates to these traits (specifically num_elements_gpu) which are wrong for Metal:

template<> struct device_type_traits<float3> {
  static const DataType data_type = TYPE_FLOAT;
  static const size_t num_elements_cpu = 4;
  static const size_t num_elements_gpu = 3;
  static_assert(sizeof(float3) == num_elements_cpu * datatype_size(data_type));
};

Ok, let's go with D13243 instead then.

In D13236#349500, @Michael Jones (michael_jones) wrote:
The specific issue that this patch is addressing relates to these traits (specifically num_elements_gpu) which are wrong for Metal:
template<> struct device_type_traits<float3> {
  static const DataType data_type = TYPE_FLOAT;
  static const size_t num_elements_cpu = 4;
  static const size_t num_elements_gpu = 3;
  static_assert(sizeof(float3) == num_elements_cpu * datatype_size(data_type));
};

I think the proper solution here is not to allow using float3 in device_memory, I'll update D13243 to do that.