Fix operator precedence consistency in ASCIIUtility.GetIndexOfFirstNonAsciiByte

[akoeplinger]

The && operator takes precedence over ||, make it explicit by wrapping in parenthesis which also makes it consistent with other usages like

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs

Line 887 in 57bfe47

if (Sse41.X64.IsSupported || (AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian))

Sse2 implies little endian so the check only needs to apply to AdvSimd.

[ghost]

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

Issue Details

---

The && operator takes precedence over || so the check was wrongly applying the little endian check only on AdvSimd.Arm64.IsSupported.

Author:	akoeplinger
Assignees:	-
Labels:	area-System.Runtime.Intrinsics
Milestone:	-

[filipnavara]

Does it make any difference? Sse2 is only x86 and that's always little endian.

[akoeplinger]

@filipnavara in theory it shouldn't make a difference however we found this while investigating an issue where this apparently lead to a miscompilation in Mono AOT+LLVM where we were calling GetIndexOfFirstNonAsciiByte_Intrinsified even though both Sse2.IsSupported and AdvSimd.Arm64.IsSupported are false which lead to an "impossible" PlatformNotSupportedException in that method

[stephentoub]

where this apparently lead to a miscompilation in Mono AOT+LLVM

Presumably that's also being fixed separately?

[akoeplinger]

Sure, if that turns out to be the issue, we're still discussing :) I just happened to see this and throw up a PR.

[jkotas]

This PR is not fixing any real bug. The code is correct as-is. If this is a workaround for a bug to unblock something, that's ok - but the PR description should describe it as such.

We have similar conditions based on the assumption that x86 is always little endian in number of other places. For example, here:

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs

Line 887 in 57bfe47

if (Sse41.X64.IsSupported || (AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian))

. Are those places also getting miscompiled?

[akoeplinger]

I was mainly doing this since the GetIndexOfFirstNonAsciiByte_Intrinsified method itself has

Debug.Assert(Sse2.IsSupported || AdvSimd.Arm64.IsSupported, "Sse2 or AdvSimd64 required.");
Debug.Assert(BitConverter.IsLittleEndian, "This SSE2/Arm64 implementation assumes little-endian.");

so my mind translated that as (Sse2 || AdvSim) && IsLittleEndian which is why the condition looked weird at first look and we also have similar code in another place in that file:

runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs

Line 1617 in 078eb57

if (BitConverter.IsLittleEndian && (Sse2.IsSupported || AdvSimd.Arm64.IsSupported))

I agree that it doesn't change the behavior after you think it through but the change makes it more consistent, is there a perf or other concern that'd apply here for why we don't want to do it?

[tannergooding]

I think the ask is to update the PR title. There is no actual error here, just a consistency/readability thing.

Since x86/x64 can only target little endian, doing if (Sse2.IsSupported || (AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian)) may also be a better option

[akoeplinger]

Sure that's fine, I'll make the two places consistent in ASCIIUtility.cs which also makes it consistent with Utf8Utility.Transcoding.cs

[jkotas]

Thanks