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

Fix hanging in JIT for ARM64 release compilation #1035

Conversation

AntonLapounov
Copy link
Member

@AntonLapounov AntonLapounov commented Apr 28, 2021

dotnet/runtime#50832 added support for Vector<nint> and Vector<nuint>, which caused release builds of JIT to hang while compiling Vector<nuint> methods. Debug builds of JIT would fail the following assertion in Compiler::raUpdateRegStateForArg:

assert(!regState->rsIsFloat);
The issue is caused by disagreement between JIT and ILCompiler on whether the aforementioned types are homogeneous vector aggregates (HVA). The fix is to extend the HVA check in VectorOfTFieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics to include IntPtr and UIntPtr element types.

The introduced IsSupportedElementType method may be temporary. When Runtime repo adds support for nint and nuint element types for Vector64<T>, Vector128<T>, and Vector256<T> types, they would have to extend similarly the TypeDesc.IsPrimitiveNumeric check here:

if (type.Context.Target.Architecture == TargetArchitecture.ARM64 &&
type.Instantiation[0].IsPrimitiveNumeric)

If they update/rename the TypeDesc.IsPrimitiveNumeric property, we will use the updated property in this class as well.

@AntonLapounov AntonLapounov added the area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation label Apr 28, 2021
@AntonLapounov AntonLapounov requested a review from jkotas April 28, 2021 18:21
@jkotas
Copy link
Member

jkotas commented Apr 28, 2021

IsPrimitiveNumeric returning false for nint/nuint looks like a oversight. @tannergooding Should we rather fix IsPrimitiveNumeric to return true for nint/nuint?

@tannergooding
Copy link
Member

Yes, this is just an oversight and I missed that it needed to also be updated.

@tannergooding
Copy link
Member

Noting that Vector64/128/256<T> don't currently support nint/nuint.

They were excluded since we have no APIs that support them today and adding that support requires API review.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2021

Yes, this is just an oversight and I missed that it needed to also be updated.

Would you mind sending PR to dotnet/runtime with the fix?

@AntonLapounov
Copy link
Member Author

Note that IsPrimitiveNumeric is also used here:

private CorInfoType getTypeForPrimitiveNumericClass(CORINFO_CLASS_STRUCT_* cls)
{
var type = HandleToObject(cls);
if (type.IsPrimitiveNumeric)
return asCorInfoType(type);
return CorInfoType.CORINFO_TYPE_UNDEF;
}

I doubt we should extend IsPrimitiveNumeric, because it would create disagreement on HVA between JIT and Crossgen2 in VectorFieldLayoutAlgorithm.

@AntonLapounov
Copy link
Member Author

Also note that the property in question is expected to mimic this one that does not accept IntPtr/UIntPtr types:

CorInfoType CEEInfo::getTypeForPrimitiveNumericClass(

@tannergooding
Copy link
Member

I doubt we should extend IsPrimitiveNumeric, because it would create disagreement on HVA between JIT and Crossgen2 in VectorFieldLayoutAlgorithm.

This sounds like is its own problem. On x64, two structs that only differ based on their usage of nint vs long should be treated identically by HVA, as they are identical at the ABI level.

If the only limiter here is that Vector128<nint> is disallowed because we don't have any public APIs taking it, then we should probably just relax that restriction as well and have it work as expected.

We will eventually have to do the same for Half, which is likewise specially treated for HFA/HVA, when we add the respective intrinsic APIs.

@tannergooding
Copy link
Member

The relevant checks in https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/simd.cpp#L607 and in the importer itself should be correctly blocking nint/nuint from matching in signatures even if getTypeForPrimitiveNumericClass returns a type.

So this should really only come down to how Vector128<nint> is treated when passed to or returned from a function.

@AntonLapounov
Copy link
Member Author

AntonLapounov commented Apr 28, 2021

If the only limiter here is that Vector128<nint> is disallowed because we don't have any public APIs taking it, then we should probably just relax that restriction as well and have it work as expected.

That should be fine; I guess that would require adding code to recognize those types in Compiler::getBaseJitTypeAndSizeOfSIMDType.

The relevant checks in https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/simd.cpp#L607 and in the importer itself should be correctly blocking nint/nuint from matching in signatures even if getTypeForPrimitiveNumericClass returns a type.

Right now they will not recognize Vector128<nint> the same way as Vector128<long> for ARM64. And the corresponding Crossgen2 check reflects that. Those two checks must be kept in sync.

@AntonLapounov
Copy link
Member Author

Closing in favor of dotnet/runtime#52016.

@AntonLapounov AntonLapounov deleted the FixArm64ReleaseHang branch April 28, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants