Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Numerics structs are incorrect sizes #6411

Closed
mellinoe opened this issue Jan 5, 2018 · 8 comments
Closed

System.Numerics structs are incorrect sizes #6411

mellinoe opened this issue Jan 5, 2018 · 8 comments
Assignees

Comments

@mellinoe
Copy link

@mellinoe mellinoe commented Jan 5, 2018

Many of the structures in the System.Numerics namespace are being padded out to an incorrectly-large size in memory. Although this was likely done as a kind of performance optimization, it has widespread side effects that break many of the main use cases for the types. This behavior is also different from .NET Core and .NET Framework.

The types affected:

Type Expected Size Actual Size
Vector2 8 16
Vector3 12 16
Plane 16 20

Plane is probably larger because it contains a Vector3, but I've listed it anyways.

There are some test cases in corefx which ensure that these structures are the expected physical size in memory. For example

@marek-safar
Copy link
Member

@marek-safar marek-safar commented Jan 5, 2018

/cc @kumpera @vargaz could you please respond

@Therzok
Copy link
Member

@Therzok Therzok commented Jan 5, 2018

Seems like the types are constrained to having at least SIMD register size.

mono/mono/metadata/class.c

Lines 1988 to 1990 in 6b58f13

/* Make SIMD types as big as a SIMD register since they can be stored into using simd stores */
if (klass->simd_type)
real_size = MAX (real_size, sizeof (MonoObject) + 16);

@akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Jan 5, 2018

I remembered this Bugzilla bug where @kumpera indicated this is expected: https://bugzilla.xamarin.com/show_bug.cgi?id=56602

@kumpera
Copy link
Member

@kumpera kumpera commented Jan 5, 2018

SIMD support was originally designed to support 16bytes only types.
The new types were added without considering this. It's something we could improve for Vector2 and Vector3.

@vargaz
Copy link
Member

@vargaz vargaz commented Jan 5, 2018

We can make them have a non-16 byte size, but then we can't do SIMD operations like loads/stores on them.

@mellinoe
Copy link
Author

@mellinoe mellinoe commented Jan 5, 2018

The size in memory of these structures is more important than individual SIMD optimizations, because there is a lot of code in the wild that works only under the assumption that these types are tightly-packed in memory. It is not a problem if the results of intermediate operations are expanded to a 16-byte form in order to better utilize SIMD operations -- this is what CoreCLR and .NET Framework do -- but the in-memory representation cannot change.

As a comparison, if this same behavior was observed in the analogous MonoGame structures, then a lot of things using them would break.

@kumpera
Copy link
Member

@kumpera kumpera commented Jan 6, 2018

@vargaz we can emit simd load/store for Vector2. Vector3 is a bit trickier since it can live at the edge of a page.

@mellinoe
Copy link
Author

@mellinoe mellinoe commented Jan 31, 2018

@kumpera @vargaz Is there any workaround for this at the moment? Some way to disable the optimizations / special codegen handling, for example? This is a pretty hard blocker for my projects working on mono.

@vargaz vargaz self-assigned this Jan 31, 2018
vargaz added a commit to vargaz/mono that referenced this issue Feb 1, 2018
…e SIMD for these types for now, since the SIMD code can't handle types

which are smaller than a SIMD reg.

Fixes mono#6411.
@vargaz vargaz closed this in #6746 Feb 2, 2018
vargaz added a commit that referenced this issue Feb 2, 2018
…e SIMD for these types for now, since the SIMD code can't handle types (#6746)

which are smaller than a SIMD reg.

Fixes #6411.
joncham added a commit to Unity-Technologies/mono that referenced this issue Apr 6, 2018
…e SIMD for these types for now, since the SIMD code can't handle types (mono#6746)

which are smaller than a SIMD reg.

Fixes mono#6411.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Apr 25, 2018
Bumps to Java.Interop/master/0afb2b0f
Bumps to llvm/master/a9cfb50e.

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=11771
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=15051
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=19436
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=45901
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=56071
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=59184
fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60065
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60225
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60298
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60359
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60568
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60756
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60848
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60862
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60900
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60904
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60986
Fixes: https://github.com/mono/mono/issues/59400
Fixes: mono/mono#6169
Fixes: mono/mono#6187
Fixes: mono/mono#6192
Fixes: mono/mono#6255
Fixes: mono/mono#6264
Fixes: mono/mono#6266
Fixes: mono/mono#6281
Fixes: mono/mono#6283
Fixes: mono/mono#6320
Fixes: mono/mono#6339
Fixes: mono/mono#6343
Fixes: mono/mono#6349
Fixes: mono/mono#6379
Fixes: mono/mono#6383
Fixes: mono/mono#6401.
Fixes: mono/mono#6411
Fixes: mono/mono#6414
Fixes: mono/mono#6490
Fixes: mono/mono#6721
Fixes: mono/mono#6767
Fixes: mono/mono#6777
Fixes: mono/mono#6848
Fixes: mono/mono#6940
Fixes: mono/mono#6948
Fixes: mono/mono#6998
Fixes: mono/mono#7016
Fixes: mono/mono#7085
Fixes: mono/mono#7086
Fixes: mono/mono#7095
Fixes: mono/mono#7137
Fixes: mono/mono#7184
Fixes: mono/mono#7240
Fixes: mono/mono#7262
Fixes: mono/mono#7289
Fixes: mono/mono#7338
Fixes: mono/mono#7356
Fixes: mono/mono#7364
Fixes: mono/mono#7378
Fixes: mono/mono#7389
Fixes: mono/mono#7460
Fixes: mono/mono#7535
Fixes: mono/mono#7536
Fixes: mono/mono#7610
Fixes: mono/mono#7624
Fixes: mono/mono#7637
Fixes: mono/mono#7655
Fixes: mono/mono#7657
Fixes: mono/mono#7685
Fixes: mono/mono#7786
Fixes: mono/mono#7792
Fixes: mono/mono#7822
Fixes: mono/mono#7860
Fixes: mono/mono#8089
Fixes: mono/mono#8267
Fixes: mono/mono#8409
Fixes: xamarin/maccore#628
Fixes: xamarin/maccore#629
Fixes: xamarin/maccore#673
Fixes: xamarin/maccore#673
Fixes: #1561
joncham added a commit to Unity-Technologies/mono that referenced this issue Jan 26, 2019
…e SIMD for these types for now, since the SIMD code can't handle types (mono#6746)

which are smaller than a SIMD reg.

Fixes mono#6411.
joncham added a commit to Unity-Technologies/mono that referenced this issue Feb 6, 2019
…e SIMD for these types for now, since the SIMD code can't handle types (mono#6746)

which are smaller than a SIMD reg.

Fixes mono#6411.
TautvydasZilys added a commit to Unity-Technologies/mono that referenced this issue Sep 12, 2019
…e SIMD for these types for now, since the SIMD code can't handle types (mono#6746)

which are smaller than a SIMD reg.

Fixes mono#6411.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants