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.

Sign up for GitHub

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

Jump to bottom

Fix HFA detection in Crossgen2 #80218

Merged
merged 2 commits into from
Jan 9, 2023
Merged

Fix HFA detection in Crossgen2 #80218

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92078b2
Fix HFA detection in Crossgen2
trylek Jan 5, 2023
9b43a8d
Add modulus checks for fields and runtime reference comment per Anton…
trylek Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 38 additions & 19 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,14 @@ public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristi
return ComputeHomogeneousAggregateCharacteristic(type);
}

/// <summary>
/// Identify whether a given type is a homogeneous floating-point aggregate. This code must be
/// kept in sync with the CoreCLR runtime method EEClass::CheckForHFA, as of this change it
/// can be found at
/// https://github.com/dotnet/runtime/blob/1928cd2b65c04ebe6fe528d4ebb581e46f1fed47/src/coreclr/vm/class.cpp#L1567
/// </summary>
/// <param name="type">Type to analyze</param>
/// <returns>HFA classification of the type parameter</returns>
private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacteristic(DefType type)
{
// Use this constant to make the code below more laconic
Expand All @@ -959,12 +967,7 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte
return NotHA;

MetadataType metadataType = (MetadataType)type;

// No HAs with explicit layout. There may be cases where explicit layout may be still
// eligible for HA, but it is hard to tell the real intent. Make it simple and just
// unconditionally disable HAs for explicit layout.
if (metadataType.IsExplicitLayout)
return NotHA;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a structure like below be considered an HFA now?

[StructLayout(LayoutKind.Explicit)]
struct Foo
{
    [FieldOffset(0)] double f1;
    [FieldOffset(5)] double f2;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Anton, you have raised a very interesting point. In the particular case you call out it won't because I think the total size will be 13 and according to line 1032 it will fail checking that the size is divisible by the floating point size, however in the loop over fields the individual fields aren't checked for the presence of proper alignment. Interestingly enough, in the original native runtime version I used for the update,

runtime/src/coreclr/vm/class.cpp

Line 1595 in 1928cd2

for (UINT i = 0; i < GetNumInstanceFields(); i++)

the check is present for R4 and R8 types but not for the general valuetype case so that, if I'm not mistaken, replacing the type of your f2 with a struct containing a single double field would be erroneously considered HFA by the runtime. As a first step, I'll update this PR by adding the modulus check to the loop, we can then discuss whether the native runtime also needs fixing or if I'm just misreading the code.

int haElementSize = 0;

switch (metadataType.Category)
{
Expand All @@ -977,12 +980,18 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte
case TypeFlags.ValueType:
// Find the common HA element type if any
ValueTypeShapeCharacteristics haResultType = NotHA;
bool hasZeroOffsetField = false;

foreach (FieldDesc field in metadataType.GetFields())
{
if (field.IsStatic)
continue;

if (field.Offset == LayoutInt.Zero)
{
hasZeroOffsetField = true;
}

// If a field isn't a DefType, then this type cannot be a HA type
if (!(field.FieldType is DefType fieldType))
return NotHA;
Expand All @@ -996,6 +1005,15 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte
{
// If we hadn't yet figured out what form of HA this type might be, we've now found one case
haResultType = haFieldType;

haElementSize = haResultType switch
{
ValueTypeShapeCharacteristics.Float32Aggregate => 4,
ValueTypeShapeCharacteristics.Float64Aggregate => 8,
ValueTypeShapeCharacteristics.Vector64Aggregate => 8,
ValueTypeShapeCharacteristics.Vector128Aggregate => 16,
_ => throw new ArgumentOutOfRangeException()
};
}
else if (haResultType != haFieldType)
{
Expand All @@ -1004,21 +1022,17 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte
// be a HA type.
return NotHA;
}

if (field.Offset.IsIndeterminate || field.Offset.AsInt % haElementSize != 0)
{
return NotHA;
}
}

// If there are no instance fields, this is not a HA type
if (haResultType == NotHA)
// If the struct doesn't have a zero-offset field, it's not an HFA.
if (!hasZeroOffsetField)
return NotHA;

int haElementSize = haResultType switch
{
ValueTypeShapeCharacteristics.Float32Aggregate => 4,
ValueTypeShapeCharacteristics.Float64Aggregate => 8,
ValueTypeShapeCharacteristics.Vector64Aggregate => 8,
ValueTypeShapeCharacteristics.Vector128Aggregate => 16,
_ => throw new ArgumentOutOfRangeException()
};

// Types which are indeterminate in field size are not considered to be HA
if (type.InstanceFieldSize.IsIndeterminate)
return NotHA;
Expand All @@ -1027,8 +1041,13 @@ private static ValueTypeShapeCharacteristics ComputeHomogeneousAggregateCharacte
// - Type of fields can be HA valuetype itself.
// - Managed C++ HA valuetypes have just one <alignment member> of type float to signal that
// the valuetype is HA and explicitly specified size.
int maxSize = haElementSize * type.Context.Target.MaxHomogeneousAggregateElementCount;
if (type.InstanceFieldSize.AsInt > maxSize)
int totalSize = type.InstanceFieldSize.AsInt;

if (totalSize % haElementSize != 0)
return NotHA;

// On ARM, HFAs can have a maximum of four fields regardless of whether those are float or double.
if (totalSize > haElementSize * type.Context.Target.MaxHomogeneousAggregateElementCount)
return NotHA;

// All the tests passed. This is a HA type.
Expand Down