Fix Issue #105820

[khushal1996]

This PR addresses #105820

Brief analysis of this issue -->

public static void Issue105820()
{
    var vr12 = (byte)0;
    if ((int)Bmi1.BitFieldExtract(3774778286U, vr12, 255) >= 0)
    {
        System.Console.WriteLine("Wrong Wrong Wrong Wrong ********************");
    }
}

Debug Output -->

Release output -->

Wrong Wrong Wrong Wrong ********************

Debug Disasm -->

; Assembly listing for method Program:Issue105820() (Tier0)
; Emitting BLENDED_CODE for X64 with AVX512 - Windows
; Tier0 code
; rbp based frame
; partially interruptible

G_M000_IG01:                ;; offset=0x0000
       55                   push     rbp
       4883EC30             sub      rsp, 48
       488D6C2430           lea      rbp, [rsp+0x30]
       33C0                 xor      eax, eax
       8945FC               mov      dword ptr [rbp-0x04], eax
 
G_M000_IG02:                ;; offset=0x000F
       33C0                 xor      eax, eax
       8945FC               mov      dword ptr [rbp-0x04], eax
       8B55FC               mov      edx, dword ptr [rbp-0x04]
       B9AE8BFEE0           mov      ecx, 0xFFFFFFFFE0FE8BAE
       41B8FF000000         mov      r8d, 255
       FF15202BF7FF         call     [System.Runtime.Intrinsics.X86.Bmi1:BitFieldExtract(uint,ubyte,ubyte):uint]
       85C0                 test     eax, eax
       7C10                 jl       SHORT G_M000_IG03
       48B980245BCECC010000 mov      rcx, 0x1CCCE5B2480
       FF153CBD1700         call     [System.Console:WriteLine(System.String)]
 
G_M000_IG03:                ;; offset=0x003C
       90                   nop      
 
G_M000_IG04:                ;; offset=0x003D
       4883C430             add      rsp, 48
       5D                   pop      rbp
       C3                   ret      
 
; Total bytes of code 67

Release disasm -->

; Assembly listing for method Program:Issue105820() (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0x0000
       4883EC28             sub      rsp, 40
 
G_M000_IG02:                ;; offset=0x0004
       B900FF0000           mov      ecx, 0xFF00
       B8AE8BFEE0           mov      eax, 0xFFFFFFFFE0FE8BAE
       C4E270F7C8           bextr    ecx, eax, ecx
       7C10                 jl       SHORT G_M000_IG04
 
G_M000_IG03:                ;; offset=0x0015
       48B9002013C066020000 mov      rcx, 0x266C0132000
       FF159B801B00         call     [System.Console:WriteLine(System.String)]
 
G_M000_IG04:                ;; offset=0x0025
       90                   nop      
 
G_M000_IG05:                ;; offset=0x0026
       4883C428             add      rsp, 40
       C3                   ret      
 
; Total bytes of code 43

Probable cause --> The test eax, eax instruction before the jl instruction is not emitted in compare to zero for instructions which write CF and OF. But the bextr instruction also falls in this category and hence ended up skipping the test instruction. This lead to wrong setting of flag during jl and hence the issue.

[dotnet-policy-service]

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

[khushal1996]

@tannergooding this fix looks safe. Ran superpmi tests and JIT suite offline. CI shows some unrelated failures but overall this looks good.

[tannergooding]

Is this "sufficient"? The comment seems to indicate we want to exactly match what test is looking for and this isn't checking that PF or ZF are written.

The general AreFlagsSetToZeroCmp is meant to determine if we can drop test x, x; and the flags may be incorrect for ZF/PF if we didn't check them as well.

[tannergooding]

Edit: Some copy/paste error happened when I was checking these, have fixed the table now

-- Given our current instructions, we only have 7 instructions which reset both overflow and carry:

• or - Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Writes_PF | Resets_CF
• and - Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Writes_PF | Resets_CF
• xor - Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Writes_PF | Resets_CF
• test - Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Writes_PF | Resets_CF
• andn - Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Resets_CF
• bextr - Resets_OF | Undefined_SF | Writes_ZF | Undefined_AF | Undefined_PF | Resets_CF
• popcnt - Resets_OF | Resets_SF | Writes_ZF | Resets_AF | Resets_PF | Resets_CF

So:

• andn violates the PF expectation of test
• bextr and popcnt violates the SF and PF expectation of test

[tannergooding]

Was a copy paste error, fixed the table of the 7 existing instructions.

Still think we need to fix to account for the other flags, except AF which shouldn't be used for test anyways since its undefined.

[khushal1996]

True. Looks like we would see this issue with andn, popcnt as well. will refine the filter further to make sure we are only skipping test for and, or and, xor.

[khushal1996]

Thanks for approving @tannergooding

[JulieLeeMSFT]

@tannergooding, should we take this fix for .NET 9?
@TIHan, please review the code.

[tannergooding]

should we take this fix for .NET 9?

I would say yes as it represents a correctness issue. That is, we generate invalid code without the fix.

This notably looks to also impact .NET 8, but we've not had any user reported issues so I would likely defer backporting to .NET 8 until a customer report comes in with a real world code example (as this pattern is likely rare and has several possible workarounds).

[JulieLeeMSFT]

Fixes #105820.

[JulieLeeMSFT]

@TIHan, please merge to .NET 10 and backport to .NET 9.

[TIHan]

/backport to release/9.0-rc1

[github-actions]

Started backporting to release/9.0-rc1: https://github.com/dotnet/runtime/actions/runs/10459546355