Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

JIT: Range Checks for "(uint)i > cns" pattern#62864

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
EgorBo merged 34 commits intodotnet:mainfromEgorBo:bound-checks-constant-len
Feb 25, 2022

Conversation

EgorBo
Copy link
Member

@EgorBoEgorBo commentedDec 15, 2021
edited
Loading

Closes#13464
Closes#1343

Example:

staticReadOnlySpan<byte>MySpan=>newbyte[]{1,2,3,4,5};intGetElement(intindex){if((uint)index<(uint)MySpan.Length)// and all variationsreturnMySpan[index];return0;}

Codegen diff:

; Method Program:GetElement(int):int:thisG_M56827_IG01:-      sub      rsp, 40G_M56827_IG02:-      mov      eax, edx-      cmp      rax, 5-      jge      SHORT G_M56827_IG05+      cmp      edx, 5+      jae      SHORT G_M56827_IG05G_M56827_IG03:-      cmp      edx, 5-      jae      SHORT G_M56827_IG07       mov      eax, edx       mov      rdx, 0xD1FFAB1E       movzx    rax, byte  ptr [rax+rdx]G_M56827_IG04:-      add      rsp, 40       ret      G_M56827_IG05:       xor      eax, eaxG_M56827_IG06:-      add      rsp, 40       ret-G_M56827_IG07:-      call     CORINFO_HELP_RNGCHKFAIL-      int3-; Total bytes of code: 51+; Total bytes of code: 25

(uint) cast is still needed for index because it can be negative. Alternative patternindex >= 0 && index < MySpan.Length also works here.

For all of these variations bound checks are now eliminated (none of them are eliminated in.NET 6.0):

publicclassTests{staticReadOnlySpan<byte>MySpan=>newbyte[]{1,2,3,4,5};publicstaticintTest1(intindex){if((uint)index<MySpan.Length)returnMySpan[index];return0;}publicstaticintTest2(intindex){if(MySpan.Length>(uint)index)returnMySpan[index];return0;}publicstaticintTest3(intindex){if(MySpan.Length<=(uint)index)return0;returnMySpan[index];}publicstaticintTest4(intindex){if((uint)index>=MySpan.Length)return0;returnMySpan[index];}publicstaticintTest5(intindex){if(index<0||index>=MySpan.Length)return0;returnMySpan[index];}publicstaticintTest6(intindex){if(index>=0&&index<MySpan.Length)returnMySpan[index];return0;}publicstaticintTest7(intindex){if(index<0)thrownewException();if(index>=MySpan.Length)thrownewArgumentException();returnMySpan[index];}publicstaticintTest8(intindex){if((uint)index<2)// 2 is less than MySpan.LengthreturnMySpan[index];return0;}publicstaticintTest9(intindex){if(index<2||index>4)// e.g. we restrict index to be in [2..3] range despite the fact MySpan is fine with [0..4]thrownewArgumentException();returnMySpan[index];}}

Unrelated pattern that got fixed too:

intfoo(uinti,int[]array){if(i<array.Length)returnarray[i];return0;}

if index is defined as unsigned - we don't need any casts to uint.

BoundedChenn31, am11, saucecontrol, En3Tho, hez2010, stephentoub, and omariom reacted with thumbs up emojigfoidl and danmoseley reacted with hooray emojidanmoseley reacted with heart emoji
@ghostghost added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labelDec 15, 2021
@ghostghost assignedEgorBoDec 15, 2021
@ghost
Copy link

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

Issue Details

Closes#13464
Closes#1343

Example:

staticReadOnlySpan<byte>MySpan=>newbyte[]{1,2,3,4,5};intGetElement(intindex){if((uint)index<MySpan.Length)returnMySpan[index];return0;}

Codegen diff:

; Method Program:GetElement(int):int:thisG_M56827_IG01:-      sub      rsp, 40G_M56827_IG02:-      mov      eax, edx-      cmp      rax, 5-      jge      SHORT G_M56827_IG05+      cmp      edx, 5+      jae      SHORT G_M56827_IG05G_M56827_IG03:-      cmp      edx, 5-      jae      SHORT G_M56827_IG07       mov      eax, edx       mov      rdx, 0xD1FFAB1E       movzx    rax, byte  ptr [rax+rdx]G_M56827_IG04:-      add      rsp, 40       ret      G_M56827_IG05:       xor      eax, eaxG_M56827_IG06:-      add      rsp, 40       ret-G_M56827_IG07:-      call     CORINFO_HELP_RNGCHKFAIL-      int3-; Total bytes of code: 51+; Total bytes of code: 25
Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo
Copy link
MemberAuthor

EgorBo commentedDec 15, 2021
edited
Loading

As a side bonus it removes bound checks for this:

staticintFoo(int[]array,inti){if((uint)i<array.Length)returnarray[i];return0;}

Previously itneeded(uint) cast forarray.Length too. But it won't work if array is Span yet (but I am planning to handle it at some point - e.g.#62421 adds[Intrinsic] forSpan.get_Length())

@EgorBo
Copy link
MemberAuthor

EgorBo commentedDec 17, 2021
edited
Loading

For all of these variations bound checks are now eliminated (none of them are eliminated in.NET 6.0):

publicclassTests{staticReadOnlySpan<byte>MySpan=>newbyte[]{1,2,3,4,5};publicstaticintTest1(intindex){if((uint)index<MySpan.Length)returnMySpan[index];return0;}publicstaticintTest2(intindex){if(MySpan.Length>(uint)index)returnMySpan[index];return0;}publicstaticintTest3(intindex){if(MySpan.Length<=(uint)index)return0;returnMySpan[index];}publicstaticintTest4(intindex){if((uint)index>=MySpan.Length)return0;returnMySpan[index];}publicstaticintTest5(intindex){if(index<0||index>=MySpan.Length)return0;returnMySpan[index];}publicstaticintTest6(intindex){if(index>=0&&index<MySpan.Length)returnMySpan[index];return0;}publicstaticintTest7(intindex){if(index<0)thrownewException();if(index>=MySpan.Length)thrownewArgumentException();returnMySpan[index];}publicstaticintTest8(intindex){if((uint)index<2)// 2 is less than MySpan.LengthreturnMySpan[index];return0;}publicstaticintTest9(intindex){if(index<2||index>4)// e.g. we restrict index to be in [2..3] range despite the fact MySpan is fine with [0..4]thrownewArgumentException();returnMySpan[index];}}

Unrelated pattern that got fixed too:

intfoo(uinti,int[]array){if(i<array.Length)returnarray[i];return0;}

if index is defined as unsigned - we don't need any casts to uint.

gfoidl, rameel, and samsosa reacted with heart emoji

@EgorBoEgorBoforce-pushed thebound-checks-constant-len branch frome87d3b2 to58f4f68CompareDecember 18, 2021 00:31
@EgorBo
Copy link
MemberAuthor

/azp run Fuzzlyn, Antigen

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBoEgorBo marked this pull request as ready for reviewDecember 20, 2021 10:47
@EgorBo
Copy link
MemberAuthor

EgorBo commentedDec 20, 2021
edited
Loading

@dotnet/jit-contrib@SingleAccretion PTAL (Antigen failures are not related)

I generated a lot of test cases locally (~100k lines of code) e.g.https://gist.github.com/EgorBo/1ba16f0d9b1b7d87044c7ed4d14db117 to validate there are no surprises. But I don't think I should add them to the repo as they are too big/slow to run and don't add much value.

@EgorBo
Copy link
MemberAuthor

EgorBo commentedDec 23, 2021
edited
Loading

@EgorBo
Copy link
MemberAuthor

@dotnet/jit-contrib PTAL

@EgorBo
Copy link
MemberAuthor

EgorBo commentedFeb 23, 2022
edited
Loading

@jakobbotsch can you take a look again? New diffs:https://dev.azure.com/dnceng/public/_build/results?buildId=1621068&view=ms.vss-build-web.run-extensions-tab

I inspected a couple of minor regressions and they seem to be caused by CSE/RA

Copy link
Member

@jakobbotschjakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, just had one question.
cc@SingleAccretion in case you want to give this a lookover as well.

EgorBoand others added3 commitsFebruary 24, 2022 23:46
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo
Copy link
MemberAuthor

@SingleAccretion thanks for the feedback!! addressed almost all of it except BashToNop - I'm leaving it to whoever will refactor it to optNarrowTree.

Failures are not related:#65818

@EgorBoEgorBo merged commite4dde6b intodotnet:mainFeb 25, 2022
@EgorBoEgorBo deleted the bound-checks-constant-len branchFebruary 25, 2022 17:40
@ghostghost locked asresolvedand limited conversation to collaboratorsMar 27, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@SingleAccretionSingleAccretionSingleAccretion left review comments

@jakobbotschjakobbotschjakobbotsch approved these changes

Assignees

@EgorBoEgorBo

Labels
area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

Span indexer doesn't elide bounds check when span initialized from an RVA static JIT doesn't eliminate bounds checks on const strings
4 participants
@EgorBo@jakobbotsch@SingleAccretion@JulieLeeMSFT

[8]ページ先頭

©2009-2025 Movatter.jp