Fix CAS mustExpand assertions in checked build

[am11]

On community platforms, clrjit checked builds are running into:

ILC: /root/dotnet/runtime/src/coreclr/jit/importercalls.cpp:4857
ILC: Assertion failed '!"Unhandled must expand intrinsic, throwing PlatformNotSupportedException"' in 'System.Threading.Interlocked:Exchange(byref,ushort):ushort' during 'Importation' (IL size 8; hash 0x23160c7c; MinOpts)

https://godbolt.org/z/5Wxz89W4r (@fuad1502's example from #113250)
Note that this is NativeAOT specific issue, crossgen2 and regular corehost apps don't seem to have this problem https://godbolt.org/z/M7eaaT7aT

[am11]

cc @sunlijun-610, @dotnet/samsung, @MichalPetryka

[jakobbotsch]

You cannot just choose to not expand a must-expand intrinsic. That's the whole point of must-expand.

It seems like the C# implementation should be adjusted to avoid this being a must-expand intrinsic when not supported.

[am11]

This is specific to AOT ABI, crossgen2 and corehost form factors seem to be fine without this patch. It seems we do alter mustExpand in the AOT block above

runtime/src/coreclr/jit/importercalls.cpp

Line 3390 in d174205

if (IsTargetAbi(CORINFO_NATIVEAOT_ABI))

[jakobbotsch]

Yes, on NAOT certain more intrinsics are must-expand. That's not a problem.

Not expanding a must-expand intrinsic is a problem. Must expand means that the C# implementation is a self-recursive call. It is going to result in hang at runtime to not expand this.

[jakobbotsch]

It's not clear to me why JIT is seing a self-recursive implementation. From what I can tell, ushort version of Exchange should not be must-expand for LA64/RISCV64:

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs

Lines 116 to 153 in d174205

	/// <summary>Sets a 16-bit signed integer to a specified value and returns the original value, as an atomic operation.</summary>
	/// <param name="location1">The variable to set to the specified value.</param>
	/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
	/// <returns>The original value of <paramref name="location1"/>.</returns>
	/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
	[Intrinsic]
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	[CLSCompliant(false)]
	public static ushort Exchange(ref ushort location1, ushort value)
	{
	#if ((MONO && (TARGET_AMD64 || TARGET_ARM64 || TARGET_WASM)) || !MONO && (TARGET_X86 || TARGET_AMD64 || TARGET_ARM64))
	return Exchange(ref location1, value); // Must expand intrinsic
	#else
	// this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object
	nuint offset = Unsafe.OpportunisticMisalignment(ref location1, sizeof(uint));
	ref uint alignedRef = ref Unsafe.As<ushort, uint>(ref Unsafe.SubtractByteOffset(ref location1, offset));
	int bitOffset =
	(int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(ushort)) * 8); // to bit offset
	Debug.Assert(bitOffset is 0 or 16);
	uint mask = ~((uint)ushort.MaxValue << bitOffset);
	uint shiftedValue = (uint)value << bitOffset;
	// this doesn't need to be volatile since CompareExchange will update stale values
	uint originalValue = alignedRef;
	uint newValue;
	do
	{
	// make sure the ref is still aligned
	Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)));
	newValue = originalValue & mask | shiftedValue;
	} while (originalValue !=
	(originalValue = CompareExchange(ref alignedRef, newValue, originalValue)));
	// verify the GC hasn't broken the ref
	Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As<uint, ushort>(ref alignedRef), ref location1) == offset);
	return (ushort)(originalValue >> bitOffset);
	#endif
	}

That would be the thing to figure out here.

[am11]

It could be that caller of

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs

Line 495 in d174205

public static unsafe T CompareExchange<T>(ref T location1, T value, T comparand)

will run into recursion from line 501 because ILC rewrites generic to non-generic:

runtime/src/coreclr/tools/Common/TypeSystem/IL/Stubs/InterlockedIntrinsics.cs

Line 57 in d174205

MethodDesc compareExchangeNonGeneric = method.OwningType.GetKnownMethod("CompareExchange",

[am11]

BTW, I wasn't able to reproduce it locally. Perhaps @hez2010 could confirm this but it seems like godbolt is using x64 runtime libs (specifically corelib) for all ilc --targetarch values? e..g --targetarch arm (32-bit) fails differently than riscv64 https://godbolt.org/z/996aMTdE1.

[hez2010]

For ilc godbolt is using x64 corelibs for all archs because setting up the cross-build of aotsdk for other archs is not trivial.
We could potentially improve the godbolt dotnet build script to support aotsdk cross-build for each arch. Current we only build corelib for cases other than naot: https://github.com/compiler-explorer/dotnet-builder/blob/41570b24c82d4437c190d959097408c71e090e38/build/build.sh#L97-L102
And the place where we pass the aotsdk to ilc: https://github.com/compiler-explorer/compiler-explorer/blob/5f8c801b7f90cb44aa22bdfa385b94cdf855a0c4/lib/compilers/dotnet.ts#L922

[am11]

Thanks for the clarification. The code @jakobbotsch pointed out lead to the theory that it's arch mismatch, glad that the mystery is resolved. :)

We could potentially improve the godbolt dotnet build script to support aotsdk cross-build for naot: compiler-explorer/dotnet-builder@41570b2/build/build.sh#L97-L102

It sounds like we could build clr.nativeaotlibs subset for all desktop platforms (except linux-x86), but I'm not familiar with their build budget as it would likely increase the build time in a significant way.

[hez2010]

but I'm not familiar with their build budget as it would likely increase the build time in a significant way

I don't think you need to concern about the build budget. There are already many compilers that require much more build time than dotnet. See https://github.com/compiler-explorer/compiler-workflows/actions, where clang/llvm takes more than an hour to build everyday.

[am11]

@tomeksowi, this turned out to be the godbolt only issue. Your suggestion about small size GT_XCHG on riscv64 is something that can be looked into separately along with

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs

Lines 116 to 153 in d174205

	/// <summary>Sets a 16-bit signed integer to a specified value and returns the original value, as an atomic operation.</summary>
	/// <param name="location1">The variable to set to the specified value.</param>
	/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
	/// <returns>The original value of <paramref name="location1"/>.</returns>
	/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
	[Intrinsic]
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	[CLSCompliant(false)]
	public static ushort Exchange(ref ushort location1, ushort value)
	{
	#if ((MONO && (TARGET_AMD64 || TARGET_ARM64 || TARGET_WASM)) || !MONO && (TARGET_X86 || TARGET_AMD64 || TARGET_ARM64))
	return Exchange(ref location1, value); // Must expand intrinsic
	#else
	// this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object
	nuint offset = Unsafe.OpportunisticMisalignment(ref location1, sizeof(uint));
	ref uint alignedRef = ref Unsafe.As<ushort, uint>(ref Unsafe.SubtractByteOffset(ref location1, offset));
	int bitOffset =
	(int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(ushort)) * 8); // to bit offset
	Debug.Assert(bitOffset is 0 or 16);
	uint mask = ~((uint)ushort.MaxValue << bitOffset);
	uint shiftedValue = (uint)value << bitOffset;
	// this doesn't need to be volatile since CompareExchange will update stale values
	uint originalValue = alignedRef;
	uint newValue;
	do
	{
	// make sure the ref is still aligned
	Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)));
	newValue = originalValue & mask | shiftedValue;
	} while (originalValue !=
	(originalValue = CompareExchange(ref alignedRef, newValue, originalValue)));
	// verify the GC hasn't broken the ref
	Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As<uint, ushort>(ref alignedRef), ref location1) == offset);
	return (ushort)(originalValue >> bitOffset);
	#endif
	}