Add Simd matrix types and use them to bind native Simd matrices. Fixes #58599 - #2571
Conversation
The OpenTK matrices (Matrix2/3/4) has a different memory layout than Apple's matrix_float2x2/matrix_float3x3/matrix_float4x4 matrices: the OpenTK versions are row-major, while Apple's versions are column-major. This means that when blitting memory from one to the other, the matrix will appear to have been transposed. See bug #[58599][2] for an example. Unfortunately we've already bound several API with Apple's matrix_floatXxX matrices using OpenTK's matrices, and although they're bound wrong, and the matrices appear transposed on input/output, we can't change the behavior because we have to maintain compatibility. This means we have to introduce new API, and then also just define new matrix types with the right memory layout (column-major). Additionally, in the future we might be able to mark these matrices as Simd-matrixes, so that the JIT/AOT compiler can generate the correct alignment for them, avoiding having to define a native conversion method (using the [MarshalDirective] hack). The matrices have been designed to match API-wise the matrices in the System.Numerics.Vectors namespace [1], but for the moment with only a few basic operations implemented (Determinant/Transpose/Multiply) defined (we can always add more later if we want to). In addition explicit conversion operators to and from the corresponding OpenTK.MatrixX are implemented, which means any operation defined in the OpenTK matrices can be used by converting back and forth. [1]: https://msdn.microsoft.com/en-us/library/system.numerics.matrix4x4(v=vs.111).aspx [2]: https://bugzilla.xamarin.com/show_bug.cgi?id=58599
|
A question that remains is whether we should add corresponding Simd vector types too. Especially Vector3 is troublesome, because the native simd version is 16 bytes, since there's 4 bytes of padding at the end (but if we do one, I'd say we do all, just to be consistent). The padding in the native Vector3 type leads to easy-to-make mistakes such as this: PR #2565. One option is to add Simd vector types and only use them in any new frameworks (ARKit/Vision). |
| return !left.Equals (right); | ||
| } | ||
|
|
||
| public static explicit operator global::OpenTK.Matrix2 (MatrixFloat2x2 value) |
There was a problem hiding this comment.
Question: should this be an implicit operator instead? The conversion is lossless (it's just transposing the matrix), but it could also lead to slow code (it would be very easy to write code that does a lot of conversions, which would slow it down).
There was a problem hiding this comment.
No, it's too easy to hide the performance cost. The overuse of the explicit one should be spotted in code reviews as a potential "non-free" issue.
The implementation changed, and these stayed accidentally.
|
Build failure |
spouliot
left a comment
spouliot
left a comment
There was a problem hiding this comment.
I need to read it again (lots of stuff) but tests makes me feel better. There's a few things in xtro that needs to be consistent with the existing tests.
|
|
||
| if (contains_simd_types && very_strict) { | ||
| // We can't map this method to a native function. | ||
| Console.WriteLine ($"!simd-can't-map-managed! {method}"); |
There was a problem hiding this comment.
start with either !unknown- or !wrong- so it gets sorted along other important/critical logs
There was a problem hiding this comment.
This is not important/critical, so I didn't change it.
There was a problem hiding this comment.
it does sounds like an unknown that requires investigation (like an API that won't work) unlike missing that have not indirect impact (i.e. what we ship is correct but incomplete)
| ManagedSimdInfo existing; | ||
| if (managed_methods.TryGetValue (key, out existing)) { | ||
| if (very_strict) | ||
| Console.WriteLine ($"!simd-double-mapping! same key '{key}' for both '{existing.Method}' and '{method}'"); |
There was a problem hiding this comment.
start with either !unknown- or !wrong- so it gets sorted along other important/critical logs
There was a problem hiding this comment.
This is not important/critical, so I didn't change it.
There was a problem hiding this comment.
let's reuse existing tags so they are grouped together, e.g. !duplicate-simd-mapping
| var typeName = type.ToString (); | ||
|
|
||
| if (!rv && typeName.Contains ("simd")) | ||
| Console.WriteLine ($"!simd-unknown-type! Could not detect that {typeName} is a Simd type, but its name contains 'simd'."); |
There was a problem hiding this comment.
start with either !unknown- or !wrong- so it gets sorted along other important/critical logs
| } | ||
|
|
||
| if (IsExtVector (type, ref simd_type)) | ||
| Console.WriteLine ($"!simd-unmapped-type! The Simd type {simd_type} does not have a mapping. Please add one in SimdCheck.cs"); |
There was a problem hiding this comment.
start with either !unknown- or !wrong- so it gets sorted along other important/critical logs
| if (!anyCalls) | ||
| return; | ||
|
|
||
| Console.WriteLine ($"!simd-missing-marshaldirective! {method}: simd type: {simd_type}"); |
There was a problem hiding this comment.
start with either !unknown- or !wrong- so it gets sorted along other important/critical logs
| return; | ||
| if (!strict) | ||
| return; | ||
| Console.WriteLine ($"!simd-can't-map-native! {decl}: could not find a managed method (selector: {decl.Selector} name: {decl.GetName ()}. Found the simd type '{simd_type}' in the native signature."); |
There was a problem hiding this comment.
start with either !unknown- or !wrong- so it gets sorted along other important/critical logs
| CheckMarshalDirective (method, simd_type); | ||
|
|
||
| // We have a potentially broken managed method. This needs fixing/investigation. | ||
| Console.WriteLine ($"!unknown-simd-type-in-signature! {method}: the native signature has a simd type ({simd_type}), while the corresponding managed method is using an incorrect (non-simd) type."); |
There was a problem hiding this comment.
That was unintentional 😄
There was a problem hiding this comment.
must have been a natural choice :)
|
Build failure |
…raries. Fixes this build problem for framework-test: clang: error: linker command failed with exit code 1 (use -v to see invocation) MTOUCH: error MT5212: Native linking failed, duplicate symbol: '_x_mdltransform_get_rotation_matrix'. MTOUCH: error MT5213: Duplicate symbol in: /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/framework-test/obj/iPhoneSimulator/Debug-unified/mtouch-cache/XStaticObjectTest.framework/XStaticObjectTest (Location related to previous error) MTOUCH: error MT5213: Duplicate symbol in: /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/framework-test/obj/iPhoneSimulator/Debug-unified/mtouch-cache/XStaticArTest.framework/XStaticArTest(libtest-ar.x86_64.o) (Location related to previous error) MTOUCH: error MT5212: Native linking failed, duplicate symbol: '_x_mdltransformcomponent_get_local_transform'. MTOUCH: error MT5212: Native linking failed, duplicate symbol: '_x_mdltransform_create_global_transform'. MTOUCH: error MT5212: Native linking failed, duplicate symbol: '_x_get_matrix_float4x4'. MTOUCH: error MT5212: Native linking failed, duplicate symbol: '_x_get_matrix_float3x3'. MTOUCH: error MT5212: Native linking failed, duplicate symbol: '_x_get_matrix_float2x2'. MTOUCH: error MT5209: Native linking error: 6 duplicate symbols for architecture x86_64 MTOUCH: error MT5202: Native linking failed. Please review the build log.
…attributes need to state that too.
…d ignore the right simd matrix types.
Fixes this introspection/Mac problem:
***** ApiSignatureTest.NativeSignatures
Selector: uniformWithName:matrixFloat2x2: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat2x2, nothing encoded
Selector: uniformWithName:matrixFloat3x3: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat3x3, nothing encoded
Selector: uniformWithName:matrixFloat4x4: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat4x4, nothing encoded
Selector: setMatrixFloat2x2Value: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat2x2, nothing encoded
Selector: setMatrixFloat3x3Value: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat3x3, nothing encoded
Selector: setMatrixFloat4x4Value: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat4x4, nothing encoded
Selector: initWithName:matrixFloat2x2: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat2x2, nothing encoded
Selector: initWithName:matrixFloat3x3: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat3x3, nothing encoded
Selector: initWithName:matrixFloat4x4: on type SpriteKit.SKUniform, Type: Simd.MatrixFloat4x4, nothing encoded
… build work.
Fixes this:
Simd/MatrixFloat4x4Test.cs(28,12): error CS0117: 'Asserts' does not contain a definition for 'AreEqual'
Simd/MatrixFloat4x4Test.cs(29,12): error CS0117: 'Asserts' does not contain a definition for 'AreEqual'
Simd/MatrixFloat4x4Test.cs(37,12): error CS0117: 'Asserts' does not contain a definition for 'AreEqual'
Simd/MatrixFloat4x4Test.cs(48,12): error CS0117: 'Asserts' does not contain a definition for 'AreEqual'
[...]
…xammac tests.
Exclude code that requires the test binding project from xammac tests because
there's currently no XM version of the test binding project.
Fixes this:
tests/monotouch-test/SpriteKit/UniformTest.cs(20,7): error CS0246: The type or namespace name 'Bindings' could not be found (are you missing a using directive or an assembly reference?)
tests/monotouch-test/ModelIO/MDLMaterialProperty.cs(37,7): error CS0246: The type or namespace name 'Bindings' could not be found (are you missing a using directive or an assembly reference?)
tests/monotouch-test/ModelIO/MDLTransform.cs(34,7): error CS0246: The type or namespace name 'Bindings' could not be found (are you missing a using directive or an assembly reference?)
rolfbjarne
left a comment
rolfbjarne
left a comment
There was a problem hiding this comment.
@spouliot I've fixed some of the xtro messages, but not those I didn't consider important/critical (so that they're not sorted with the important/critical logs).
|
|
||
| if (contains_simd_types && very_strict) { | ||
| // We can't map this method to a native function. | ||
| Console.WriteLine ($"!simd-can't-map-managed! {method}"); |
There was a problem hiding this comment.
This is not important/critical, so I didn't change it.
| ManagedSimdInfo existing; | ||
| if (managed_methods.TryGetValue (key, out existing)) { | ||
| if (very_strict) | ||
| Console.WriteLine ($"!simd-double-mapping! same key '{key}' for both '{existing.Method}' and '{method}'"); |
There was a problem hiding this comment.
This is not important/critical, so I didn't change it.
| CheckMarshalDirective (method, simd_type); | ||
|
|
||
| // We have a potentially broken managed method. This needs fixing/investigation. | ||
| Console.WriteLine ($"!unknown-simd-type-in-signature! {method}: the native signature has a simd type ({simd_type}), while the corresponding managed method is using an incorrect (non-simd) type."); |
There was a problem hiding this comment.
That was unintentional 😄
|
Build success |
|
@spouliot I've fixed all the xtro messages now. |
spouliot
left a comment
spouliot
left a comment
There was a problem hiding this comment.
-
Introspection tests needs to be adjusted (failures due to unencoded simd types).
-
SKTransformNodeis a new type and should not add an obsolete API
[Obsolete ("Use 'RotationMatrix3x3' instead.")]
public virtual OpenTK.Matrix3 RotationMatrix { get; set; }
|
|
||
| if (contains_simd_types && very_strict) { | ||
| // We can't map this method to a native function. | ||
| Console.WriteLine ($"!simd-can't-map-managed! {method}"); |
There was a problem hiding this comment.
it does sounds like an unknown that requires investigation (like an API that won't work) unlike missing that have not indirect impact (i.e. what we ship is correct but incomplete)
|
Build success |
|
intro ran fine on the last build |
…broken (and obsolete) non-simd version of RotationMatrix.
|
Build success |
5 participants
The OpenTK matrices (Matrix2/3/4) has a different memory layout than Apple's
matrix_float2x2/matrix_float3x3/matrix_float4x4 matrices: the OpenTK versions
are row-major, while Apple's versions are column-major.
This means that when blitting memory from one to the other, the matrix will
appear to have been transposed.
See bug #58599 for an example.
Unfortunately we've already bound several API with Apple's matrix_floatXxX
matrices using OpenTK's matrices, and although they're bound wrong, and the
matrices appear transposed on input/output, we can't change the behavior
because we have to maintain compatibility.
This means we have to introduce new API, and then also just define new matrix
types with the right memory layout (column-major). Additionally, in the future
we might be able to mark these matrices as Simd-matrixes, so that the JIT/AOT
compiler can generate the correct alignment for them, avoiding having to
define a native conversion method (using the [MarshalDirective] hack).
The matrices have been designed to match API-wise the matrices in the
System.Numerics.Vectors namespace 1, but for the moment with only a few
basic operations implemented (Determinant/Transpose/Multiply) defined (we can
always add more later if we want to).
In addition explicit conversion operators to and from the corresponding
OpenTK.MatrixX are implemented, which means any operation defined in the
OpenTK matrices can be used by converting back and forth.
Also add a new xtro check that verifies that we bind things correctly from now
on + that all simd API have [MarshalDirective] attributes.