Use PackedIndexOfIsSupported checks in more places

[radekdoulik]

This should avoid the size regression on WebAssembly and possibly other platforms without Sse2.

The regression is side effect of #78861 which uses PackedSpanHelpers.CanUsePackedIndexOf (!!T) and TShouldUsePacked.Value to guard the usage of PackedSpanHelpers.

Because these involve generics, illinker is unable to link the PackedSpanHelpers type away and that pulls other parts in, like System.Runtime.Intrinsics.X86.* types. See https://gist.github.com/radekdoulik/c0b52247d472f69bcf983ade78a924ea for more complete list.

This change gets us back 9,216 bytes in the case of app used to repro the regression.

...
  -             Type System.PackedSpanHelpers
  -             Type System.Runtime.Intrinsics.X86.X86Base
  -             Type System.Runtime.Intrinsics.X86.Sse
  -             Type System.Runtime.Intrinsics.X86.Sse2
Summary:
  -       9,216 File size -0.76% (of 1,215,488)
  -       2,744 Metadata size -0.43% (of 636,264)
  -           4 Types count

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[radekdoulik]

@vitek-karas @sbomer, do you think the linker might be able to evaluate the checks involving generics in future?

[MihaZupan]

do you think the linker might be able to evaluate the checks involving generics in future?

It would be nice if at least cases like

runtime/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Packed.cs

Lines 26 to 31 in 7f93bd6

	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	public static unsafe bool CanUsePackedIndexOf<T>(T value) =>
	PackedIndexOfIsSupported &&
	RuntimeHelpers.IsBitwiseEquatable<T>() &&
	sizeof(T) == sizeof(ushort) &&
	*(ushort*)&value - 1u < 254u;

could be handled given that PackedIndexOfIsSupported is the first condition we check.

[vitek-karas]

I'll look into it a bit - I don't the generics are the problem. I think the problem is that our analysis is not smart enough to figure out that if the PackedIndexOfIsSupported is always false, then the subsequent conditions will not be evaluated. And evaluation of the subsequent conditions probably fails because it either can't figure out their value or it thinks it may have side effects (especially the unsafe pointer code would probably make the analysis really nervous trying to reason about it). But it's just a guess - I'll have to look into in detail.

[vargaz]

This is what the linker produces for CanUsePackedIndexOf:

  .method public hidebysig static bool  CanUsePackedIndexOf<T>(!!T A_0) cil managed aggressiveinlining
  {
    // Code size       5 (0x5)
    .maxstack  8
    IL_0000:  ldc.i4.0
    IL_0001:  brfalse.s  IL_0003

    IL_0003:  ldc.i4.0
    IL_0004:  ret
  } // end of method PackedSpanHelpers::CanUsePackedIndexOf

[MihaZupan]

@radekdoulik looking at your diff of methods and types, it looks like the linker was already able to strip out the TrueConst, indicating that it's already capable of following the generic CanUsePackedIndexOf.

There are two usage patterns here:

1. The one in SpanHelpers.T.cs that always check the generic CanUsePackedIndexOf
   • If my above assumption is correct, these should already be fine and don't need the extra check
2. The ones in IndexOfAny1CharValue.cs, IndexOfAny2CharValues.cs, IndexOfAny3CharValues.cs, and IndexOfAnyCharValuesInRange.cs where we rely on the IRuntimeConst.
   • Here the realization the linker would have to make is that it already stripped out TrueConst, leaving only one implementation of IRuntimeConst. In this case, it could remove all IRuntimeConst.Value checks as being false.
   • Assuming it can't do that, I take it the only extra checks we need in this PR are in those 4 files (making those in IndexOfAnyValues.cs and SpanHelpers.T.cs redundant).

[radekdoulik]

That might be possible, I first added the checks to CanUsePackedIndexOf and when that didn't improve I added the checks to TShouldUsePacked.Value. I will try to remove the ones next to CanUsePackedIndexOf to confirm that.

[vitek-karas]

Here the realization the linker would have to make is that it already stripped out TrueConst, leaving only one implementation of IRuntimeConst. In this case, it could remove all IRuntimeConst.Value checks as being false.

This is very difficult to implement - it requires analyzing the entire app to figure out that TrueConst is not needed, remove it and then go back and "fixup" all callsites which can now assume a const false and propagate that and so on. It's not impossible, just difficult and would probably have non-trivial implications on the perf of the tool. If we still need trimming to work around those places, can we wrap it with the if for the feature switch - I know it's redundant from runtime's point of view, but there's only so much trimming can do.

Also of note - these kind of optimizations are currently treated as that - just optimizations, so the tools are free to "give up" on them if it's too difficult. It also means that different tools will have different behaviors. For example, I would not be surprised if NativeAOT didn't work the same way in this particular case.

@sbomer is probably going to look into defining the minimum bar for feature switch based trimming which would then become a guaranteed functionality in the tools, but we don't have that yet. Also - expect the min bar to be pretty minimum.

[radekdoulik]

I have tried version with removed changes in IndexOfAnyValues.cs and SpanHelpers.T.cs. After that the illinker wasn't able to get rid of CanUsePackedIndexOf method and thus the PackedSpanHelpers type was not removed completely as well. So these checks still improve the size a bit.

[radekdoulik]

The timeout is probably #79255