Replaced flag checking expressions by Enum.HasValue

[YohDeadfall]

What does the pull request do?

The PR replaces expression checking the value for a flag by invocation of Enum.HasFlag which isn't boxing since 2017 (dotnet/runtime#6080).

What is the current behavior?

Currently there are a lot of hand written expressions checking for null which can be simplified.

What is the updated/expected behavior with this PR?

No changes except more clean code.

[grokys]

We added Enum.HasFlagCustom because Enum.HasFlag didn't seem to work non-boxing in a lot of cases - see #3066

I'm not sure if this is still the case? @MarchingCube @jkoritzinsky

[jkoritzinsky]

The non-boxing optimization I believe only happens on Tier 1 JIT or R2R, so we added HasFlagCustom to ensure that we don’t box with Tiered Compilation enabled.

[MarchingCube]

Also all debug builds are boxing, we technically also support Mono and .Net Framework and addition of tiered compilation that also boxes initially makes me always suspicious when I see Enum.HasFlag.

[YohDeadfall]

@AndyAyersMS @EgorBo, could you confirm that tiered compilation affects this optimization and tier 0 may box an enum value?

@grokys, @MarchingCube, in case if it's true (I hope that this optimization works always) then it makes sense to fix your implementation which is slow, unsafe and buggy (having a non-int enum may cause at least an incorrect result or even crash the app).

[EgorBo]

@AndyAyersMS @EgorBo, could you confirm that tiered compilation affects this optimization and tier 0 may box an enum value?

@grokys, @MarchingCube, in case if it's true (I hope that this optimization works always) then it makes sense to fix your implementation which is slow, unsafe and buggy (having a non-int enum may cause at least an incorrect result or even crash the app).

it's not optimized in tier0 currently (it's below this check)
I guess it's quite slow for tier0 and can affect throughput/startup time (see gtOptimizeEnumHasFlag).

[AndyAyersMS]

I guess it's quite slow

It's not a particularly slow optimization at jit time, if that's what you meant by "slow". Currently, Tier0 and MinOpts are identical, and we don't want to do optimizations at MinOpts, so that has held us back from doing optimizations at Tier0.

However I think we should change that and break Tier0 and MinOpts apart, and then enable opts like this for Tier0 (along with other intrinsic and boxing-related opts); see dotnet/runtime#9120.

None of that would change the behavior seen with debug codegen, or the behavior seen for .Net Framework, so not sure it really addresses the concerns above.

[kekekeks]

it makes sense to fix your implementation which is slow, unsafe and buggy (having a non-int enum may cause at least an incorrect result or even crash the app).

SL

        [SharpLab.Runtime.JitGeneric(typeof(StringSplitOptions))]
        [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
        public static unsafe bool HasFlagCustom<T>(this T value, T flag) where T : unmanaged, Enum
        {
            var intValue = *(int*)&value;
            var intFlag = *(int*)&flag;

            return (intValue & intFlag) == intFlag;
        }

x64

C.HasFlagCustom[[System.StringSplitOptions, System.Private.CoreLib]](System.StringSplitOptions, System.StringSplitOptions)
    L0000: and ecx, edx
    L0002: cmp ecx, edx
    L0004: sete al
    L0007: movzx eax, al
    L000a: ret

I don't see what exactly is slow for those instructions that only operate with registers. Legacy framework JIT generates pointer-based stack access, but that's still better than default HasFlag.

Regarding non-int enums: we simply do not use that function for non-int enums.

[YohDeadfall]

Regarding non-int enums: we simply do not use that function for non-int enums.

At least it can be changed to be safe and contain a throw expression if non-int enums are not allowed.

[kekekeks]

At least it can be changed to be safe and contain a throw expression if non-int enums are not allowed.

Unfortunately if(Marshal.SizeOf<T>() == 4) check doesn't get resolved at JIT-time and generates invocations at run-time. .NET type system also doesn't allow us to have enum size type constraint.

[YohDeadfall]

Unsafe.SizeOf<T> works at JIT time and is supported by any target framework (see dependencies).

[YohDeadfall]

The following code is safe and produces the same instructions as existing.

public static bool HasFlagCustom<T>(this T value, T flag) where T : unmanaged, Enum
{
    if (Unsafe.SizeOf<T>() != Unsafe.SizeOf<int>())
        throw new NotSupportedException();
            
    var valueUntyped = Unsafe.As<T, int>(ref value);
    var flagUntyped = Unsafe.As<T, int>(ref flag);
            
    return (valueUntyped & flagUntyped) == flagUntyped;
}

[kekekeks]

For some reason checking the size (for all enum sizes) reduces the generated code quality:

C.HasFlagWithCheck[[Ienum, _]](Ienum, Ienum)
    L0000: sub rsp, 0x18
    L0004: xor eax, eax
    L0006: mov [rsp+8], rax
    L000b: mov [rsp+0x10], rax
    L0010: mov [rsp+0x20], ecx
    L0014: mov [rsp+0x28], edx
    L0018: mov eax, [rsp+0x20]
    L001c: mov edx, [rsp+0x28]
    L0020: and eax, edx
    L0022: cmp eax, edx
    L0024: sete al
    L0027: movzx eax, al
    L002a: add rsp, 0x18
    L002e: ret

C.HasFlagCustomOld[[Ienum, _]](Ienum, Ienum)
    L0000: and ecx, edx
    L0002: cmp ecx, edx
    L0004: sete al
    L0007: movzx eax, al
    L000a: ret

[kekekeks]

Your proposed variant actually works better on the legacy framework than our current one though. I wonder if we should have separate method versions for different enum sizes to overcome that JIT limitation.

[kekekeks]

Mkay, replacing Unsafe.SizeOf<T>() with sizeof(T) fixes the code, but then changing it to use Unsafe.As adds weirdness again.

[AndyAyersMS]

For some reason checking the size ... reduces the generated code quality:

Can you open a bug for this over on dotnet/runtime? The jit may be getting confused by the parts of the IL that end up not getting imported.

[kekekeks]

This version seems to consistently produce optimized code with both .NET core and legacy framework JIT

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static unsafe bool HasFlagWithCheck<T>(this T value, T flag) where T : unmanaged, Enum
        {
            if(sizeof (T) == 1)
            {            
                var byteValue = Unsafe.As<T, byte>(ref value);
                var byteFlag = Unsafe.As<T, byte>(ref flag);
                return (byteValue & byteFlag) == byteFlag;
            }
            else if(sizeof (T) == 2)
            {
                var shortValue = Unsafe.As<T, short>(ref value);
                var shortFlag = Unsafe.As<T, short>(ref flag);
                return (shortValue & shortFlag) == shortFlag;
            }
            else if(sizeof (T) == 4)
            {
                var intValue = Unsafe.As<T, int>(ref value);
                var intFlag = Unsafe.As<T, int>(ref flag);
                return (intValue & intFlag) == intFlag;
            }
            else if (sizeof (T) == 8)
            {
                var longValue = Unsafe.As<T, long>(ref value);
                var longFlag = Unsafe.As<T, long>(ref flag);
                return (longValue & longFlag) == longFlag;
            }
            else
                throw new NotSupportedException("Enum with size of " + Unsafe.SizeOf<T>() + " are not supported");
        }

C.HasFlagWithCheck[[Lenum, _]](Lenum, Lenum)
    L0000: and rcx, rdx
    L0003: cmp rcx, rdx
    L0006: setz al
    L0009: movzx eax, al
    L000c: ret

C.HasFlagWithCheck[[Ienum, _]](Ienum, Ienum)
    L0000: and ecx, edx
    L0002: cmp ecx, edx
    L0004: setz al
    L0007: movzx eax, al
    L000a: ret

[kekekeks]

@AndyAyersMS
dotnet/runtime#43509

[grokys]

What is the status on this PR?

[YohDeadfall]

Since there's only int sized enums, I think I will just support them only for better code gen if you're okay with it. Perhaps, today will submit changes.

[YohDeadfall]

Accidentally dropped mine branch while cleaning up mine fork (git branch -a gave me endless list of branches). Going to open a new one soon.