Use architecture independent code paths in Array

[joncham]

We at Unity have in-progress work towards a modern .NET ecosystem enabled by adopting CoreCLR in lieu of Mono and updating IL2CPP to support the .NET BCL. I am investigating how architecture specific code paths can be reduced if not completely removed in the .NET BCL. I understand this architecture agnostic approach is not the direction of .NET in general, but I am hoping to validate how to best approach this without maintaining a completely separate fork.

The current BCL, more specifically System.Private.Corlib, has a non-trivial number of preprocessor checks for architecture/bit specific code. At Unity, our IL2CPP AOT compiler has historically consumed the Mono BCL which was architecture agnostic. This enabled a number of benefits for us when targeting multiple architectures such as reduced iteration time via a single managed linker step and transpile step to C++, and reduced deployment size via sharing our binary metadata file between architectures (produced during transpile).

In this preliminary PR, I include two commits with possible approaches that I wanted to validate before submitting larger PRs. In the first commit, native sized integers (nint & nuint) are used rather than a preprocessor check and int & long types. In the second commit, the int & long types remain in use but are switched via a IntPtr.Size check.

A few questions:

1. Any preference on either commit as a general approach when possible?
2. What metrics should be evaluated when making such changes? Are there sufficient performance and size tests run as part of the automated PR process?
3. In more complex scenarios where the preprocessor checks are used, does a new architecture independent target, e.g. TARGET_ARCH_NEUTRAL, make sense to introduce and use alongside the existing code paths? I would expect this path to contain both implementations with a runtime check similar to commit 2 in this PR.

Thanks for any feedback!

[jkotas]

Suggested change

	result = GenericBinarySearch<nint>(array, adjustedIndex, length, value);
	break;
	case CorElementType.ELEMENT_TYPE_U:
	result = GenericBinarySearch<nuint>(array, adjustedIndex, length, value);
	result = (IntPtr.Size == 4) ? GenericBinarySearch<int>(array, adjustedIndex, length, value) : GenericBinarySearch<long>(array, adjustedIndex, length, value);
	break;
	case CorElementType.ELEMENT_TYPE_U:
	result = (IntPtr.Size == 4) ? GenericBinarySearch<uint>(array, adjustedIndex, length, value) : GenericBinarySearch<ulong>(array, adjustedIndex, length, value);

Same pattern as below - avoids nint/nuint generic instantiations.

[jkotas]

Could you please update all #if TARGET_32/64BIT in Array.cs to this pattern? We can merge this PR after that.

[joncham]

I pushed change to take this approach uniformly in this file.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[jkotas]

Any preference on either commit as a general approach when possible?

I think the runtime check approach is better in this specific case. It avoids code size hit from extra generic instantiations. (The check will be turned into no-op via trimmer substitutions or by the compiler, so it is not really a runtime check.)

What metrics should be evaluated when making such changes?

Perf (throughput, memory consumption, code size) and maintainability.

Are there sufficient performance and size tests run as part of the automated PR process?

Our perf and size tests run in outer loop. They do not run as part of the PR. We typically depend on target perf tests for PR validation as necessary.

In more complex scenarios where the preprocessor checks are used, does a new architecture independent target, e.g. TARGET_ARCH_NEUTRAL, make sense to introduce and use alongside the existing code paths

I think the arch-neutral config will be leaving some perf on the table. A good example is: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs . I do not think there is a good way to make the arch-neutral variant that is perf-neutral.

New TARGET_ARCH_NEUTRAL may help with readability of the code in some situations.

cc @stephentoub

[EgorBo]

The check will be turned into no-op via trimmer substitutions or by the compiler, so it is not really a runtime check

ILLink still struggles with some, e.g. #83169 (still a constant in the end, but affects inlining decisions/dll size).

[jkotas]

Looking at some of the long lines, I have realized that we can just use goto case

[am11]

TARGET_ARCH_NEUTRAL

Does this aim to cover only differences in bitness (i.e., 32-bit vs. 64-bit), or would it also encompass architecture types (e.g., x86/x64 vs. ARM/ARM64)?