Skip to content

Fix CAS mustExpand assertions in checked build #113300

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

Closed
am11 wants to merge 6 commits into from
Closed

Fix CAS mustExpand assertions in checked build #113300

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d0c588c
Fix CAS mustExpand assertions in checked build
am11 Mar 9, 2025
d223101
fmt
am11 Mar 9, 2025
4638f09
Fix linux-x86 build
am11 Mar 9, 2025
882f9f9
Update comments
am11 Mar 9, 2025
cc4be06
Update importercalls.cpp
am11 Mar 9, 2025
c76e3c1
Handle LA64 differently
am11 Mar 9, 2025
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
30 changes: 24 additions & 6 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4112,22 +4112,31 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd,
}
#endif // defined(TARGET_ARM64) || defined(TARGET_RISCV64)

#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_RISCV64)
#if defined(TARGET_64BIT) || defined(TARGET_X86)
// TODO-ARM-CQ: reenable treating InterlockedCmpXchg32 operation as intrinsic
case NI_System_Threading_Interlocked_CompareExchange:
{
#if defined(TARGET_LOONGARCH64)
// GT_CMPXCHG is NYI in lsraloongarch64
mustExpand = false;
break;
#endif

var_types retType = JITtype2varType(sig->retType);

if (genTypeSize(retType) > TARGET_POINTER_SIZE)
{
break;
}
#if !defined(TARGET_XARCH) && !defined(TARGET_ARM64)
#if defined(TARGET_RISCV64)
// CAS for small types is natively supported starting with the Zabha extension; however,
// hardware support for RVA23 profile is not available at the time of writing.
else if (genTypeSize(retType) < 4)
{
mustExpand = false;

@jakobbotsch jakobbotsch Mar 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 am11 Mar 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 jakobbotsch Mar 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 jakobbotsch Mar 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 am11 Mar 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 am11 Mar 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 hez2010 Mar 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 am11 Mar 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 hez2010 Mar 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

break;
}
#endif // !defined(TARGET_XARCH) && !defined(TARGET_ARM64)
#endif // defined(TARGET_RISCV64)

if ((retType == TYP_REF) &&
(impStackTop(1).val->IsIntegralConst(0) || impStackTop(1).val->IsIconHandle(GTF_ICON_OBJ_HDL)))
Expand Down Expand Up @@ -4158,18 +4167,27 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd,
case NI_System_Threading_Interlocked_Exchange:
case NI_System_Threading_Interlocked_ExchangeAdd:
{
#if defined(TARGET_LOONGARCH64)
// GT_XADD and GT_XCHG are NYI in lsraloongarch64
mustExpand = false;
break;
#endif

var_types retType = JITtype2varType(sig->retType);

if (genTypeSize(retType) > TARGET_POINTER_SIZE)
{
break;
}
#if !defined(TARGET_XARCH) && !defined(TARGET_ARM64)
#if defined(TARGET_RISCV64)
// CAS for small types is natively supported starting with the Zabha extension; however,
// hardware support for RVA23 profile is not available at the time of writing.
else if (genTypeSize(retType) < 4)
{
mustExpand = false;
break;
}
#endif // !defined(TARGET_XARCH) && !defined(TARGET_ARM64)
#endif // defined(TARGET_RISCV64)

if ((retType == TYP_REF) &&
(impStackTop().val->IsIntegralConst(0) || impStackTop().val->IsIconHandle(GTF_ICON_OBJ_HDL)))
Expand Down Expand Up @@ -4199,7 +4217,7 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd,
callType, op1, op2);
break;
}
#endif // defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_RISCV64)
#endif // defined(TARGET_64BIT) || defined(TARGET_X86)

case NI_System_Threading_Interlocked_MemoryBarrier:
{
Expand Down