ILLink substitute BitConverter.IsLittleEndian as true constant

[EgorBo]

There is a method in Ascii.Utility.cs (SPC.dll):

private static bool FirstCharInUInt32IsAscii(uint value)
{
    return (BitConverter.IsLittleEndian && (value & 0xFF80u) == 0)
        || (!BitConverter.IsLittleEndian && (value & 0xFF800000u) == 0);
}

After ILLink it looks like this:

  .method private hidebysig static bool 
      FirstCharInUInt32IsAscii(unsigned int32 'value') cil managed
  {
    .maxstack 8
    IL_0000: ldsfld       bool System.BitConverter::IsLittleEndian
    IL_0005: pop 
    IL_0006: ldarg.0      
    IL_0007: ldc.i4       65408
    IL_000c: and
    IL_000d: brfalse.s    IL_0018
    IL_000f: ldsfld       bool System.BitConverter::IsLittleEndian
    IL_0014: brtrue.s     IL_0016
    IL_0016: ldc.i4.0
    IL_0017: ret
    IL_0018: ldc.i4.1
    IL_0019: ret
  }

which is roughly:

private static bool FirstCharInUInt32IsAscii(uint value)
{
    int unused = BitConverter.IsLittleEndian;
    if ((value & 65408) == 0)
        goto RetTrue;
    if (BitConverter.IsLittleEndian)
        ; 
    return false;
RetTrue:
    return true;
}

So while it seems that ILLink did touch this method (thanks to #37615) it seems that it could do a better job here by replacing ldsfld bool System.BitConverter::IsLittleEndian with just ldc.i4.1, etc.

This doesn't let JIT to inline this small (in fact) method, because JIT doesn't resolve ldsfld tokens during IL prescan and it doesn't know that it's a special IsLittleEndian field that is always a constant) - it can be found in e.g. GetIndexOfFirstNonAsciiChar_Intrinsified

[ghost]

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

Issue Details

---

There is a method in Ascii.Utility.cs (SPC.dll):

private static bool FirstCharInUInt32IsAscii(uint value)
{
    return (BitConverter.IsLittleEndian && (value & 0xFF80u) == 0)
        || (!BitConverter.IsLittleEndian && (value & 0xFF800000u) == 0);
}

After ILLink it looks like this:

  .method private hidebysig static bool 
      FirstCharInUInt32IsAscii(unsigned int32 'value') cil managed
  {
    .maxstack 8
    IL_0000: ldsfld       bool System.BitConverter::IsLittleEndian
    IL_0005: pop 
    IL_0006: ldarg.0      
    IL_0007: ldc.i4       65408
    IL_000c: and
    IL_000d: brfalse.s    IL_0018
    IL_000f: ldsfld       bool System.BitConverter::IsLittleEndian
    IL_0014: brtrue.s     IL_0016
    IL_0016: ldc.i4.0
    IL_0017: ret
    IL_0018: ldc.i4.1
    IL_0019: ret
  }

which is roughly:

private static bool FirstCharInUInt32IsAscii(uint value)
{
    int unused = BitConverter.IsLittleEndian;
    if ((value & 65408) == 0)
        goto RetTrue;
    if (BitConverter.IsLittleEndian)
        ; 
    return false;
RetTrue:
    return true;
}

So while it seems that ILLink did touch this method (thanks to #37615) it seems that it could do a better job here by replacing ldsfld bool System.BitConverter::IsLittleEndian with just ldc.i4.1, etc.

This doesn't let JIT to inline this small (in fact) method, because JIT doesn't resolve ldsfld tokens during IL prescan and it doesn't know that it's a special IsLittleEndian field that is always a constant) - it can be found in e.g. GetIndexOfFirstNonAsciiChar_Intrinsified

Author:	EgorBo
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

There is a method in Ascii.Utility.cs (SPC.dll):

private static bool FirstCharInUInt32IsAscii(uint value)
{
    return (BitConverter.IsLittleEndian && (value & 0xFF80u) == 0)
        || (!BitConverter.IsLittleEndian && (value & 0xFF800000u) == 0);
}

After ILLink it looks like this:

  .method private hidebysig static bool 
      FirstCharInUInt32IsAscii(unsigned int32 'value') cil managed
  {
    .maxstack 8
    IL_0000: ldsfld       bool System.BitConverter::IsLittleEndian
    IL_0005: pop 
    IL_0006: ldarg.0      
    IL_0007: ldc.i4       65408
    IL_000c: and
    IL_000d: brfalse.s    IL_0018
    IL_000f: ldsfld       bool System.BitConverter::IsLittleEndian
    IL_0014: brtrue.s     IL_0016
    IL_0016: ldc.i4.0
    IL_0017: ret
    IL_0018: ldc.i4.1
    IL_0019: ret
  }

which is roughly:

private static bool FirstCharInUInt32IsAscii(uint value)
{
    int unused = BitConverter.IsLittleEndian;
    if ((value & 65408) == 0)
        goto RetTrue;
    if (BitConverter.IsLittleEndian)
        ; 
    return false;
RetTrue:
    return true;
}

So while it seems that ILLink did touch this method (thanks to #37615) it seems that it could do a better job here by replacing ldsfld bool System.BitConverter::IsLittleEndian with just ldc.i4.1, etc.

This doesn't let JIT to inline this small (in fact) method, because JIT doesn't resolve ldsfld tokens during IL prescan and it doesn't know that it's a special IsLittleEndian field that is always a constant) - it can be found in e.g. GetIndexOfFirstNonAsciiChar_Intrinsified

Author:	EgorBo
Assignees:	-
Labels:	untriaged, area-Tools-ILLink
Milestone:	-

[jkotas]

Is linker keeping the ldsfld around to preserve the cctor trigger side effect? Preserving these side-effects is important for some substitutions.

[jkotas]

JIT doesn't resolve ldsfld tokens during IL prescan and it doesn't know that it's a special IsLittleEndian field that is always a constant)

Should the JIT start doing that in Tier1 or AOT modes to give the right treatment to initialized readonly fields?

[vitek-karas]

If it's the access to the substituted field (and not a propagated constant) I think it should be OK to remove the ldsfld completely. But I haven't debugged the linker to tell exactly why it's keeping the instruction - it could be just general "conservative" approach.

[EgorBo]

JIT doesn't resolve ldsfld tokens during IL prescan and it doesn't know that it's a special IsLittleEndian field that is always a constant)

Should the JIT start doing that in Tier1 or AOT modes to give the right treatment to initialized readonly fields?

I'll take a look

[marek-safar]

Is linker keeping the ldsfld around to preserve the cctor trigger side effect?

Correct, we didn't put any effort yet to teach linker how to trim static constructors bodies and for that reason we also keep anything that could static ctor even when applying the substitutions.

[EgorBo]

Is linker keeping the ldsfld around to preserve the cctor trigger side effect?

Correct, we didn't put any effort yet to teach linker how to trim static constructors bodies and for that reason we also keep anything that could static ctor even when applying the substitutions.

It seems that even if I remove static constructor from BitConverter (IsLittleEndian = true; is not needed since that field is an intrinsic for CoreCLR/Crossgen/NAOT) - it still doesn't remove the field

[EgorBo]

Ah, the lambda in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/BitConverter.cs#L672 still keeps it 😞 - presumably, can be replaced with string.FastAllocate