Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Add support for bounds check no throw assertions in range check and make overflow check more precise #100777

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Apr 8, 2024

Fix #9422
Fix #83349

Example:

[MethodImpl(MethodImplOptions.NoInlining)]
public static int Foo(int i, int[] indices)
{
    while ((uint)i < (uint)indices.Length)
    {
        i = indices[i];
    }

    return i;
}

Before:

G_M9328_IG02:  ;; offset=0x0004
       mov      eax, dword ptr [rdx+0x08]
       cmp      eax, ecx
       jbe      SHORT G_M9328_IG04
       align    [0 bytes for IG03]
						;; size=7 bbWeight=1 PerfScore 3.25

G_M9328_IG03:  ;; offset=0x000B
       cmp      ecx, eax
       jae      SHORT G_M9328_IG06
       mov      ecx, ecx
       mov      ecx, dword ptr [rdx+4*rcx+0x10]
       cmp      eax, ecx
       ja       SHORT G_M9328_IG03
						;; size=14 bbWeight=4 PerfScore 19.00

G_M9328_IG04:  ;; offset=0x0019
       mov      eax, ecx
						;; size=2 bbWeight=1 PerfScore 0.25

G_M9328_IG05:  ;; offset=0x001B
       add      rsp, 40
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25

G_M9328_IG06:  ;; offset=0x0020
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
						;; size=6 bbWeight=0 PerfScore 0.00
; Total bytes of code: 38

After:

G_M9328_IG02:  ;; offset=0x0004
       mov      eax, dword ptr [rdx+0x08]
       cmp      eax, ecx
       jbe      SHORT G_M9328_IG04
       align    [0 bytes for IG03]
						;; size=7 bbWeight=1 PerfScore 3.25
G_M9328_IG03:  ;; offset=0x000B
       mov      ecx, ecx
       mov      ecx, dword ptr [rdx+4*rcx+0x10]
       cmp      eax, ecx
       ja       SHORT G_M9328_IG03
						;; size=10 bbWeight=4 PerfScore 14.00
G_M9328_IG04:  ;; offset=0x0015
       mov      eax, ecx
						;; size=2 bbWeight=1 PerfScore 0.25
G_M9328_IG05:  ;; offset=0x0017
       add      rsp, 40
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 28, prolog size 4, PerfScore 19.00, instruction count 12, allocated bytes for code 28 (MethodHash=e6badb8f) for method Program:Foo(int,int[]):int (FullOpts)
; ============================================================

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 8, 2024
@jakobbotsch
Copy link
Member Author

This PR in its current state still doesn't fix the duplicated bounds checks in the shared generic version of GetValueRefOrAddDefault, so this needs some more work/investigation.

@jakobbotsch
Copy link
Member Author

For some reason, the int i = bucket - 1 that exists before the loop seems to deter range check from eliminating the bounds check. We compute the correct range, but then give up anyway because of that subtraction of 1:

Merging assertions from pred edges of BB07 for op [000074] $401
   Computed Range [000074] => <0, $346 + -1>
}
Does overflow [000074]?
Does overflow [000526]?
Does overflow [000536]?
Does overflow [000085]?
[000085] does not overflow
[000536] does not overflow
Does overflow [000534]?
Does overflow [000054]?
Does overflow [000052]?
[000052] does not overflow
Does overflow [000053]?
[000053] does not overflow
Checking bin op overflow ADD <Unknown, Unknown> <-1, -1>
[000054] overflows
[000534] overflows
[000526] overflows
[000074] overflows
Method determined to overflow.

No idea what range check is trying to do here, why is this "can overflow" check even necessary if we have proved a range through other means? I suppose range check internally reasons without considering overflow, then later only uses the result if it can prove no overflow is possible. But clearly that's conservative here.

A new minimal repro that does not manage to get rid of bounds checks looks like:

[MethodImpl(MethodImplOptions.NoInlining)]
public static int Foo(ref int startIndex, int[] indices)
{
    int i = startIndex - 1;
    while ((uint)i < (uint)indices.Length)
    {
        i = indices[i];
    }

    return i;
}

@EgorBo
Copy link
Member

EgorBo commented Apr 9, 2024

Yep, it's not the first time I see "CanOverflow" ruining things becuase of being too conservative 🤷

@jakobbotsch
Copy link
Member Author

DoesOverflow does not utilize assertions at all while proving this. It should be able to stop following SSA defs as soon as it sees relevant assertions about the local. That should fix the issue.
But it's weird to me that these checks are done as two separate steps. It seems like it would be more natural to make the range computation conservative when it detects the possibility of overflow.

@jakobbotsch
Copy link
Member Author

It should be able to stop following SSA defs as soon as it sees relevant assertions about the local.

Not quite. Even if we have an assertion for a particular local's value we still may have computed a tighter range based on the local's definition, so if that computation didn't take into account the possibility of overflow then we still need to examine its def.
So we probably need an additional check that the computed range contains the full range determined by the assertion. Not very elegant.

On a side note ComputeRange does seem to take the possibility of overflow into account when calculating ranges, so I am unsure why DoesOverflow is needed at all. I'll try disabling it and investigating some diffs...

@EgorBo
Copy link
Member

EgorBo commented Apr 9, 2024

I'll try disabling it and investigating some diffs...

I remember I tried that in the past. I think we even discussed possible cases why it currently cannot be removed, probably @SingleAccretion remembers

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch jakobbotsch changed the title JIT: Add support for bounds check no throw assertions in range check JIT: Add support for bounds check no throw assertions in range check and make overflow check more precise Apr 10, 2024
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS @EgorBo

Diffs.
I think it should be fine to remove the overflow check entirely, but I need to do more due diligence before doing that. What range check is doing here is very relevant to some of the stuff I'll need to do for strength reduction as well, so I think the time will be well spent regardless.
In the meantime this PR makes the overflow check a bit smarter by avoiding chasing through SSA defs when we get to a path where we can prove (using assertions) that the value is in the final range we reported. When we can prove that it seems like it should be safe to report that no preceding overflow could have impacted any decision we make based on the information.

A few minor regressions which happen because using this new assertion means we may end up computing a range that gives an upper bound in terms of a different array. That might cause us to no longer eliminate some bounds checks on other arrays.

@jakobbotsch jakobbotsch marked this pull request as ready for review April 10, 2024 19:00
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

@EgorBo
Copy link
Member

EgorBo commented Apr 11, 2024

I think it should be fine to remove the overflow check entirely,

I found this from the discussion we had in the past about it:

SingleAccretion — 03/29/2023 9:25 AM
it is needed for the "monotonically increasing" part to be correct.
So it's supposed to prevent issues like #69735 (comment).

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Apr 11, 2024

I think it should be fine to remove the overflow check entirely,

I found this from the discussion we had in the past about it:

SingleAccretion — 03/29/2023 9:25 AM it is needed for the "monotonically increasing" part to be correct. So it's supposed to prevent issues like #69735 (comment).

Thanks for pointing to that! I'll add a test for something like that to #100820 so we at least have some coverage. So it seems like the overflow handling during the range computation just isn't complete. This example computes the following:

BinOp add ranges <Dependent, $c3 + -1> <2, 2> = <Dependent, $c3 + 1>
            Computed Range [000030] => <Dependent, $c3 + 1>

I think this should rather compute to <Unknown, Unkonwn> unless we can prove that $c3 + 1 never overflows.

@jakobbotsch
Copy link
Member Author

FWIW, I want to study the example above a bit more carefully before I merge this one.

@jakobbotsch
Copy link
Member Author

I think it's fine. The range returned by range check without the overflow check may have an incorrect lower bound for the monotonically increasing cases; but assertions are sufficient on those paths to make the lower bound right anyway.

@jakobbotsch jakobbotsch merged commit 69110bf into dotnet:main Apr 17, 2024
245 of 249 checks passed
@jakobbotsch jakobbotsch deleted the fix-9422 branch April 17, 2024 10:00
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
4 participants