-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Test failure JIT\\HardwareIntrinsics\\Arm\\AdvSimd.Arm64\\AdvSimd.Arm64_Part2_r\\AdvSimd.Arm64_Part2_r.cmd #65281
Comments
Here is a simplest program I managed to create to reproduce the issue (also pushed it to my clone of the repo - 14bce9a) // Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System.Runtime.InteropServices;
using System.Runtime.Intrinsics;
class Runtime_63856
{
[StructLayout(LayoutKind.Auto)]
struct int8x16x2
{
Vector128<byte> _0;
Vector128<byte> _1;
}
struct Wrapper
{
int8x16x2 fld;
}
static int Main()
{
var val = default(Wrapper);
return 100;
}
} Note that When running on Arm64 with
this will fail the following way
|
@AaronRobinsonMSFT @davidwrighton if this is a new failure is it related to the ref fields work? |
@davidwrighton is this something you could look into since its related to intrinsics? |
This isn't intrinsics, this is struct layout, but fortunately enough while this failure is enough to break the test, it shouldn't actually break customers. However, lack of testing of the intrinsics is a problem so it does need to be fixed. |
Ok, this is actually a general purpose AutoLayout bug impacting the alignment calculations for structures. We were improperly computing a the wrong alignment in a number of scenarios. (Sometimes too high, sometimes too low.). Given the general lack of autolayout structures, this probably isn't a terribly impacting bug, but it likely does reduce the set of functions that can be used out of an R2R image if ValueTuple is used, and for applications which hold ValueTuple within the version bubble of the app, larger problems may arise. (Also its a problem if ValueTuple starts being used in CoreLib.) |
- In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8. - The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8) In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime. - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now. Fixes dotnet#65281
…73738) * New test * Fix auto layout algorithm to compute structure alignment correctly - In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8. - The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8) In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime. - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now. Fixes #65281 Co-authored-by: Tomas Rylek <trylek@microsoft.com>
…In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8. - The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8) In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime. - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now. Fixes #65281
…nment correctly (#74091) * New test * Fix auto layout algorithm to compute structure alignment correctly - In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8. - The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8) In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime. - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now. Fixes #65281 * Re-enable disabled test * Remove file that shouldn't be added as part of the new test * Make a few test types public to silence unassigned field errors * Update comments and add more testing Co-authored-by: David Wrighton <davidwr@microsoft.com> Co-authored-by: Tomas Rylek <trylek@microsoft.com>
Run: runtime-coreclr outerloop 20220213.2
Failed test:
Error message:
NOTE:
AdvSimd.Arm64\AdvSimd.Arm64_Part2_r
andAdvSimd.Arm64\AdvSimd.Arm64_Part2_ro
are disabled by #65227.Please re-enable these tests when the issue is fixed!
The text was updated successfully, but these errors were encountered: