Intrinsify Array GetArrayDataReference for SZ arrays

[MichalPetryka]

Handles SZ arrays passed to the Array typed GetArrayDataReference via the intrinsic path too.

This should help JIT with avoiding UB caused by the C# implementation doing Unsafe.As on the array which can cause invalid codegen in some cases. Also makes the codegen a bit better.

[ghost]

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

Issue Details

---

Handles SZ arrays passed to the Array typed GetArrayDataReference via the intrinsic path too.

Author:	MichalPetryka
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[JulieLeeMSFT]

@MichalPetryka, can you describe the issue or link the issue?

[MichalPetryka]

@MichalPetryka, can you describe the issue or link the issue?

Described the motivation in the readme, worth noting is that I don't see any diffs in SPMI but I've seen examples of such code in the runtime, so I need to debug whether the type information is propagated correctly when inlining.

[MichalPetryka]

@MihuBot

[EgorBo]

@MichalPetryka So apparently jit-diff found zero diffs too, so can you explain again why exactly we need this, can you come up with a snippet that does a wrong thing and this PR fixes?

[MichalPetryka]

@MichalPetryka So apparently jit-diff found zero diffs too, so can you explain again why exactly we need this, can you come up with a snippet that does a wrong thing and this PR fixes?

It found some diffs now, the motivation here is that Unsafe.As on the array can confuse VN and cause some invalid codegen, I can't find any sample that reproduces with the MD GADR though since the original ones from @SingleAccretion seem to not be problematic here/

[EgorBo]

It found some diffs now

they don't look related, in fact it must be an unrelated issue @jakobbotsch fixed in #88385

[MichalPetryka]

It found some diffs now

they don't look related, in fact it must be an unrelated issue @jakobbotsch fixed in #88385

They are related, look at the static method at the bottom of the file, not at the instance one.

[EgorBo]

They are related, look at the static method at the bottom of the file, not at the instance one.

can you share it here? I don't see any diffs except the if <-> cmove ones

[MichalPetryka]

They are related, look at the static method at the bottom of the file, not at the instance one.

can you share it here? I don't see any diffs except the if <-> cmove ones

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/PointerArrayMarshaller.cs

Line 198 in 6d74424

return ref MemoryMarshal.GetArrayDataReference(array);

[EgorBo]

They are related, look at the static method at the bottom of the file, not at the instance one.

can you share it here? I don't see any diffs except the if <-> cmove ones

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/PointerArrayMarshaller.cs

Line 198 in 6d74424

return ref MemoryMarshal.GetArrayDataReference(array);

So, basically, it improves just one method accross all libs, right? GetPinnableReference (multiple generic versions of it because that's how PMI spawns them) - cc @dotnet/jit-contrib any thoughts about where is the line when we take changes with close to zero-diffs?

[tannergooding]

So, basically, it improves just one method accross all libs, right?

The API in question is relatively new and is intended to be used in high perf scenarios. The BCL not having light-up from it doesn't mean that external users won't benefit, particularly in the places where some libraries use MD arrays more prominently.

any thoughts about where is the line when we take changes with close to zero-diffs?

I think we should be taking changes, like this, that explicitly improve these "built-in"/"foundational" high-performance helper APIs, particularly when its done via +24, -7 lines of code change and just reutilizing some existing logic we already have and where it resolves a known issue with undefined-behavior that we've seen hit in its sibling API. -- I would think that ideally this would actually be a little more complex and also handle MD arrays.

We ultimately don't have tons of these lowlevel APIs with most being already covered. We're also not adding them in a way that isn't "pay for play". This recognition is all done on import and only for the APIs in question so the actual impact to the JIT is minimal and there isn't a lot of reason to not do them when they're this small.

[EgorBo]

The API in question is relatively new and is intended to be used in high perf scenarios.

I doubt this non-generic Array overload is so, it looks more like a workaround for T*[] case

The BCL not having light-up from it doesn't mean

Nobody ever stated the otherwise, but in case of zero diff we need a solid proof it makes sense. It also means that the test coverage is only the test Michal just added (that also doesn't look complete - e.g. I don't see shared generic case).

where it resolves a known issue with undefined-behavior that we've seen hit in its sibling API.

Would be nice to see that UB in action, so far it was stated that this overload is unlikely to hit it.

Overall I am ok taking this change, just wanted to raise a question regarding JIT changes without clear benefits/which don't fix any issues

[EgorBo]

@MichalPetryka thanks!