Skip to content

Revert unintentional alignment change for Vector256<T> #36673

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

Merged
AntonLapounov merged 1 commit into from
May 19, 2020
Merged

Revert unintentional alignment change for Vector256<T> #36673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions src/coreclr/src/tools/Common/Compiler/VectorFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ public override bool ComputeContainsGCPointers(DefType type)

public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristics(DefType type)
{
if (type.Context.Target.Architecture == TargetArchitecture.ARM64)
if (type.Context.Target.Architecture == TargetArchitecture.ARM64 &&
type.Instantiation[0].IsPrimitiveNumeric)
Copy link
Member

@tannergooding tannergooding May 18, 2020

Choose a reason for hiding this comment

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

Should this be a special IsSupportedVectorBaseType instead? We only support the 10 primitive types today, but that may change in the future when we add System.Half for example (which is a supported vector type but which isn't an IL primitive).

Copy link
Member Author

@AntonLapounov AntonLapounov May 19, 2020

Choose a reason for hiding this comment

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

That follows naming of the API, getTypeForPrimitiveNumericClass is used to recognize those 10 allowed primitive types here:

runtime/src/coreclr/src/jit/simd.cpp

Line 580 in e5badd7

CorInfoType type = info.compCompHnd->getTypeForPrimitiveNumericClass(baseTypeHnd);

If we add more allowed types, we should rename the API and this property at the same time.

Copy link
Member

@MichalStrehovsky MichalStrehovsky May 19, 2020

Choose a reason for hiding this comment

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

I assume that the fact we don't support this with nint and nuint (IntPtr/UIntPtr) is intentional.

Copy link
Member

@tannergooding tannergooding May 19, 2020

Choose a reason for hiding this comment

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

It is currently intentional but there is a proposal to extend support to those types as well: #36160

Copy link
Member Author

@AntonLapounov AntonLapounov May 19, 2020

Choose a reason for hiding this comment

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

Then we might want to throw a RequiresRuntimeJitException for Vector*<nint> types to avoid a breaking change (passing in X vs. Q registers) later.

{
return type.InstanceFieldSize.AsInt switch
{
Expand All @@ -106,8 +107,9 @@ public static bool IsVectorType(DefType type)
{
Copy link
Member

@tannergooding tannergooding May 18, 2020

Choose a reason for hiding this comment

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

What will happen for say Vector64<bool> or Vector128<Guid> which are legal IL but unsupported by the JIT/runtime?

Copy link
Member Author

@AntonLapounov AntonLapounov May 19, 2020

Choose a reason for hiding this comment

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

Their alignment will be the same as for numeric Vector64<T> and Vector128<T> types. However, on ARM64 they will not be considered HFA/HVA and will be passed in X registers instead of Q registers. That is how VM and JIT work at present and Crossgen2 must be compatible.

return type.IsIntrinsic &&
type.Namespace == "System.Runtime.Intrinsics" &&
((type.Name == "Vector64`1") || (type.Name == "Vector128`1")) &&
type.Instantiation[0].IsPrimitive;
(type.Name == "Vector64`1" ||
type.Name == "Vector128`1" ||
type.Name == "Vector256`1");
}
}
}
19 changes: 3 additions & 16 deletions src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1759,23 +1759,10 @@ private CorInfoType getTypeForPrimitiveNumericClass(CORINFO_CLASS_STRUCT_* cls)
{
var type = HandleToObject(cls);

switch (type.Category)
{
case TypeFlags.Byte:
case TypeFlags.SByte:
case TypeFlags.UInt16:
case TypeFlags.Int16:
case TypeFlags.UInt32:
case TypeFlags.Int32:
case TypeFlags.UInt64:
case TypeFlags.Int64:
case TypeFlags.Single:
case TypeFlags.Double:
return asCorInfoType(type);
if (type.IsPrimitiveNumeric)
return asCorInfoType(type);

default:
return CorInfoType.CORINFO_TYPE_UNDEF;
}
return CorInfoType.CORINFO_TYPE_UNDEF;
}

private bool canCast(CORINFO_CLASS_STRUCT_* child, CORINFO_CLASS_STRUCT_* parent)
Expand Down
30 changes: 29 additions & 1 deletion src/coreclr/src/tools/Common/TypeSystem/Common/TypeDesc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public bool IsValueType

/// <summary>
/// Gets a value indicating whether this is one of the primitive types (boolean, char, void,
/// a floating point, or an integer type).
/// a floating-point, or an integer type).
/// </summary>
public bool IsPrimitive
{
Expand All @@ -198,6 +198,34 @@ public bool IsPrimitive
}
}

/// <summary>
/// Gets a value indicating whether this is one of the primitive numeric types
/// (a floating-point or an integer type).
/// </summary>
public bool IsPrimitiveNumeric
{
get
{
switch (GetTypeFlags(TypeFlags.CategoryMask))
{
case TypeFlags.SByte:
case TypeFlags.Byte:
case TypeFlags.Int16:
case TypeFlags.UInt16:
case TypeFlags.Int32:
case TypeFlags.UInt32:
case TypeFlags.Int64:
case TypeFlags.UInt64:
case TypeFlags.Single:
case TypeFlags.Double:
return true;

default:
return false;
}
}
}

/// <summary>
/// Gets a value indicating whether this is an enum type.
/// Access <see cref="UnderlyingType"/> to retrieve the underlying integral type.
Expand Down