Jit: Bounds checks are not removed in some inlined functions

Minimal reproduction:
SharpLab link

public readonly ref struct Wrapper
{
    private readonly ReadOnlySpan<byte> _buffer;

    public int Length => _buffer.Length;
    public byte this[int i] => _buffer[i];
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool BeginsWith0(Wrapper p)
{
    return (uint)p.Length > 1 && p[0] == 0;
}

public static bool BeginsWith0Caller(Wrapper a) => BeginsWith0(a);

BeginsWith0 produces the following code. The wrapper's Length and indexer are inlined and the JIT removes the bounds checks.

G_M8674_IG01:

G_M8674_IG02:
       mov      eax, dword ptr [rcx+8]
       cmp      eax, 1
       jbe      SHORT G_M8674_IG05

G_M8674_IG03:
       mov      rax, bword ptr [rcx]
       cmp      byte  ptr [rax], 0
       sete     al
       movzx    rax, al

G_M8674_IG04:
       ret      

G_M8674_IG05:
       xor      eax, eax

G_M8674_IG06:
       ret      
; Total bytes of code: 24

BeginsWith0Caller has unnecessary stack usage and the bounds checks are not removed.

G_M26510_IG01:
       sub      rsp, 56
       vzeroupper 
       xor      rax, rax
       mov      qword ptr [rsp+28H], rax

G_M26510_IG02:
       vmovdqu  xmm0, xmmword ptr [rcx]
       vmovdqu  xmmword ptr [rsp+28H], xmm0

G_M26510_IG03:
       cmp      dword ptr [rsp+30H], 1
       jbe      SHORT G_M26510_IG05

G_M26510_IG04:
       lea      rax, bword ptr [rsp+28H]
       cmp      dword ptr [rax+8], 0
       jbe      SHORT G_M26510_IG07
       mov      rax, bword ptr [rax]
       cmp      byte  ptr [rax], 0
       sete     al
       movzx    rax, al
       jmp      SHORT G_M26510_IG06

G_M26510_IG05:
       xor      eax, eax

G_M26510_IG06:
       add      rsp, 56
       ret      

G_M26510_IG07:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
; Total bytes of code: 69

category:cq
theme:value-numbering
skill-level:intermediate
cost:medium
impact:medium

I don't see any redundant bound checks in BeginsWith0Caller in master, how old is your branch?

G_M43082_IG02:
       vmovdqu  xmm0, xmmword ptr [rcx]
       vmovdqu  xmmword ptr [rsp+28H], xmm0
						;; bbWeight=1    PerfScore 3.00

G_M43082_IG03:
       cmp      dword ptr [rsp+30H], 1
       jbe      SHORT G_M43082_IG05
						;; bbWeight=1    PerfScore 2.00

G_M43082_IG04:
       lea      rax, bword ptr [rsp+28H]
       cmp      dword ptr [rax+8], 0
       jbe      SHORT G_M43082_IG07
       mov      rax, bword ptr [rax]
       cmp      byte  ptr [rax], 0
       sete     al
       movzx    rax, al
       jmp      SHORT G_M43082_IG06
						;; bbWeight=0.50 PerfScore 5.38

G_M43082_IG05:
       xor      eax, eax
						;; bbWeight=0.50 PerfScore 0.13

G_M43082_IG06:
       add      rsp, 56
       ret      
						;; bbWeight=1    PerfScore 1.25

; Total bytes of code 69

About a week old. The checks are still there in your code.

       cmp      dword ptr [rax+8], 0
       jbe      SHORT G_M43082_IG07

@EgorBo

@EgorBo you aren't output the prologue and epilogue ;)

Misses out this bit

G_M31226_IG07:
       call     CORINFO_HELP_RNGCHKFAIL
       int3

haha, indeed :)

Edit: The comment this was replying to was removed.

I'm surprised that pattern isn't optimized away.

public static bool IsTrue(bool a) => a ? true : false;

G_M27403_IG02:
       test     cl, cl
       jne      SHORT G_M27403_IG05
G_M27403_IG03:
       xor      eax, eax
       ret      
G_M27403_IG05:
       mov      eax, 1
       ret      

; Total bytes of code 13

@Thealexbarney

@Thealexbarney thanks for the report.

In the "Caller" case the local address visitor folds up the initial check into a LCL_FLD which value numbers differently than the IND(ADD(...)) in the bounds check that amounts to the same thing, so we fail to optimize.

In the base case, both trees are left as IND trees, because the base of the indirections is an implict by-ref argument, and CSE/optimization kicks in.

So presumably if we followed up on the following TODO-ADDR we'd see similar codegen in the "Caller" case.

runtime/src/coreclr/src/jit/lclmorph.cpp

Lines 797 to 809 in e771456

    
           void MorphLocalAddress(const Value& val) 
        
           { 
        
               assert(val.IsAddress()); 
        
               assert(val.Node()->TypeIs(TYP_BYREF, TYP_I_IMPL)); 
        
               assert(m_compiler->lvaVarAddrExposed(val.LclNum())); 
        
               LclVarDsc* varDsc = m_compiler->lvaGetDesc(val.LclNum()); 
        
               if (varDsc->lvPromoted || varDsc->lvIsStructField || m_compiler->lvaIsImplicitByRefLocal(val.LclNum())) 
        
               { 
        
                   // TODO-ADDR: For now we ignore promoted and "implict by ref" variables, 
        
                   // they require additional changes in subsequent phases. 
        
                   return;

Going to mark this as future as I don't know how common this sort of thing is and changes to implicit byref handling are likely to be somewhat involved.

Actually I probably have that backwards -- if we handle byrefs as in the above we'll fail to optimize both cases. The root issue is that a LCL_FLD and "equivalent" IR via IND and such won't value number the same; we can get the latter if the address math is spread over statements and/or we hit limitations like the implicit byref above.

;; lcl field version

N004 (  7,  8) [000012] ----G-------    *  JTRUE     void  
N003 (  5,  6) [000011] N---G--N-U--    \--*  LE        int    $200
N001 (  3,  4) [000043] ----G-------       +--*  LCL_FLD   int    V03 tmp2         [+8] Fseq[_buffer, _length] $1c0
N002 (  1,  1) [000010] ------------       \--*  CNS_INT   int    1 $43

;; split-up ind/add version

N003 (  3,  3) [000052] -A------R---    *  ASG       byref  $240
N002 (  1,  1) [000051] D------N----    +--*  LCL_VAR   byref  V04 tmp3         d:1 $240
N001 (  3,  3) [000048] ------------    \--*  LCL_FLD_ADDR byref  V03 tmp2         [+0] Fseq[_buffer] $240
...
N006 (  9, 12) [000056] ---XG-------       |  +--*  ARR_BOUNDS_CHECK_Rng void   <l:$348, c:$347>
N001 (  1,  1) [000049] ------------       |  |  +--*  CNS_INT   int    0 $40
N005 (  4,  4) [000055] ---XG-------       |  |  \--*  IND       int    <l:$202, c:$201>
N004 (  2,  2) [000065] -------N----       |  |     \--*  ADD       byref  $2c0
N002 (  1,  1) [000054] ------------       |  |        +--*  LCL_VAR   byref  V04 tmp3         u:1 $240
N003 (  1,  1) [000064] ------------       |  |        \--*  CNS_INT   long   8 field offset Fseq[_length] $280

Ideally [000043] and [000055] have the same VN

So perhaps would be nice to fix this in value numbering?

Current codegen for BeginsWith0Caller:

; Method C:BeginsWith0Caller(C+Wrapper):ubyte (FullOpts)
G_M47695_IG01:  ;; offset=0x0000
       sub      rsp, 40
						;; size=4 bbWeight=1 PerfScore 0.25

G_M47695_IG02:  ;; offset=0x0004
       mov      rax, bword ptr [rcx]
       mov      ecx, dword ptr [rcx+0x08]
       cmp      ecx, 1
       jbe      SHORT G_M47695_IG04
						;; size=11 bbWeight=1 PerfScore 5.25

G_M47695_IG03:  ;; offset=0x000F
       cmp      byte  ptr [rax], 0
       sete     al
       movzx    rax, al
       jmp      SHORT G_M47695_IG05
						;; size=11 bbWeight=0.50 PerfScore 3.12

G_M47695_IG04:  ;; offset=0x001A
       xor      eax, eax
						;; size=2 bbWeight=0.50 PerfScore 0.12

G_M47695_IG05:  ;; offset=0x001C
       add      rsp, 40
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25
; Total bytes of code: 33

Navigation Menu

Repository navigation

Jit: Bounds checks are not removed in some inlined functions #32414

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	void MorphLocalAddress(const Value& val)
	{
	assert(val.IsAddress());
	assert(val.Node()->TypeIs(TYP_BYREF, TYP_I_IMPL));
	assert(m_compiler->lvaVarAddrExposed(val.LclNum()));

	LclVarDsc* varDsc = m_compiler->lvaGetDesc(val.LclNum());

	if (varDsc->lvPromoted \|\| varDsc->lvIsStructField \|\| m_compiler->lvaIsImplicitByRefLocal(val.LclNum()))
	{
	// TODO-ADDR: For now we ignore promoted and "implict by ref" variables,
	// they require additional changes in subsequent phases.
	return;

Jit: Bounds checks are not removed in some inlined functions #32414

Description

Activity

EgorBo commented on Feb 16, 2020

Thealexbarney commented on Feb 16, 2020

benaadams commented on Feb 16, 2020

EgorBo commented on Feb 16, 2020

Thealexbarney commented on Feb 16, 2020

AndyAyersMS commented on Feb 19, 2020

AndyAyersMS commented on Feb 19, 2020

EgorBo commented on May 5, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions