Remove class constraint from Interlocked.{Compare}Exchange

[stephentoub]

#65184

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[stephentoub]

@MihuBot

[jkotas]

@lambdageek @steveisok Are there any Mono-specific concerns related to this change?

[lambdageek]

@lambdageek @steveisok Are there any Mono-specific concerns related to this change?

Are there new/existing tests for the throwing behavior?

If mono is green, one of two things is wrong (I'm on a phone and can't check):

1. Mono intrinsics are firing but they haven't been updated to throw if the arguments are non-primitive valuetypes, so they're working by accident
2. The intrinsics aren't firing and the new method bodies are perf regressions (possibly for jit/AOT, and almost certainly for the interpreter)

/cc @kg

[stephentoub]

Are there new/existing tests for the throwing behavior?

Yes:
https://github.com/dotnet/runtime/pull/104558/files#diff-b7ba854effe0b40e624638bc9ea70e50950841a776d09a3a1f36df3922f9efdfR397
https://github.com/dotnet/runtime/pull/104558/files#diff-b7ba854effe0b40e624638bc9ea70e50950841a776d09a3a1f36df3922f9efdfR824

If mono is green, one of two things is wrong

The mono legs are green (or at least the failures in the wasm legs are unrelated).

Are you saying something is definitely wrong, or are you saying if something were wrong and going unnoticed it would be those?

From what I could tell, mono's intrinsic implementations here already only support a subset of what the APIs do and already fall back to the managed implementations for other cases, and the throwing behavior is handled in the managed implementation. Did I misunderstand how things fit together?

[lambdageek]

The mono legs are green (or at least the failures in the wasm legs are unrelated).

Are you saying something is definitely wrong, or are you saying if something were wrong and going unnoticed it would be those?

I think we should keep an eye on the perf runs - I wonder if generic sharing will see a perf regression.
We're giving the inlining heuristic more work to do. The managed body of Interlocked.CompareExchange<T> is bigger now, so if the initial codegen doesn't remove the dead branches effectively it might inhibit inlining the callers of CompareExchange<T> into their callers.

Also startup perf on the interpreter might be affected since it doesn't do many optimizations in Tier0.

From what I could tell, mono's intrinsic implementations here already only support a subset of what the APIs do and already fall back to the managed implementations for other cases, and the throwing behavior is handled in the managed implementation. Did I misunderstand how things fit together?

No, I think it's fine.

In JIT/AOT we handle two cases: CompareExchange(ref X, X, X) where X is int, long, float, double, intptr and object; and CompareExchange(ref object, ref object, ref object, ref object) (The latter is an implementation detail of CompareExchange(ref object, object, object)). It will fall back to the managed implementation for: unsigned primitives, enums, other valuetypes, and generic sharing / gsharedvt cases (for generic sharing, the codegen will see a generic parameter with a special flag which won't match any of the cases that the intrinsic wants to handle). I think with the exception of gsharedvt, all the other cases should fold the typeof(T).IsValueType and sizeof(T) == N tests during the initial codegen. (but if it doesn't then we'd see perf regressions). (there is a third kind of generic sharing that sometimes shows up: partial generic sharing - this tries to share code for int and "enum with underlying type == int" (and long, short, byte) - I think those cases should work like gshared for reference types - all the false branches should be dropped during initial codegen)

In the interpreter we only seem to handle int and long and run the managed versions for everything else (but the interpreter also doesn't do generic sharing, so we only will see concrete instantiations). In general startup perf suffers if there's more method calls on hot paths since tier0 doesn't do as much optimization and managed->managed method calls are expensive in tier0.

tl;dr:

1. we should measure startup perf on the interpreter and see if there's any ill effects
2. we should look at the generated code for gshared and enum inputs in the JIT/AOT and verify that all the typeof(T).IsValueType and sizeof(T) == N branches are being removed in the initial codegen so that downstream optimizations aren't affected

[lambdageek]

oh, in case it wasn't clear: I don't think we should hold up this PR due to #104558 (comment) - just that we have some follow-up work

[stephentoub]

Thanks, @lambdageek.

In that case, I think the remaining blocker is just figuring out why I'm getting some invalid program exceptions with native aot :)

[MichalStrehovsky]

In that case, I think the remaining blocker is just figuring out why I'm getting some invalid program exceptions with native aot :)

Pushed out a fix for that. There was also a problem in crossgen2 but there it was just a deoptimization since we skip compiling methods with invalid IL and leave it to JIT.

[stephentoub]

In that case, I think the remaining blocker is just figuring out why I'm getting some invalid program exceptions with native aot :)

Pushed out a fix for that. There was also a problem in crossgen2 but there it was just a deoptimization since we skip compiling methods with invalid IL and leave it to JIT.

Thank you, @MichalStrehovsky! It would have taken me a while to discover the existence of TryGetIntrinsicMethodIL vs TryGetPerInstantiationIntrinsicMethodIL.

[jkotas]

LGTM!

[stephentoub]

Thanks, all.