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: IV opts relop evaluation misses combining facts to prove relops #110315

Open
jakobbotsch opened this issue Dec 2, 2024 · 2 comments · May be fixed by #110352
Open

JIT: IV opts relop evaluation misses combining facts to prove relops #110315

jakobbotsch opened this issue Dec 2, 2024 · 2 comments · May be fixed by #110352
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@jakobbotsch
Copy link
Member

I collected some stats for #110222 to figure out where strength reduction bails out.

Those stats looks like:

NonLoopUses: 30772
CouldNotInitializeCursors: 27066
IVUseMismatch: 34895
SameStep: 1652
SameStepWidened: 951
StepNonConstant: 3

For CouldNotInitializeCursors the reason is often that we fail to compute the trip count of the loop, in which case the IV use in the loop test disqualifies the loop from strength reduction. I looked at one such case, System.Reflection.Internal.EncodingHelper:DecodeUtf8, in libraries_tests.run. There we are unable to prove that the loop bound is at least 1:

STMT00047 ( ??? ... ??? )
N004 (  0,  0) [000202] DA---------                         ▌  STORE_LCL_VAR int    V14 tmp10        d:2 $VN.Void
N003 (  0,  0) [000201] -----------                         └──▌  PHI       int    $3c1
N001 (  0,  0) [000219] ----------- pred BB10                  ├──▌  PHI_ARG   int    V14 tmp10        u:3
N002 (  0,  0) [000216] ----------- pred BB09                  └──▌  PHI_ARG   int    V14 tmp10        u:1 $40
  => <L00, 0, 1>
  L00 exits when:
  <L00, 1, 1> >= V20.1
  Does not overflow past the test
  Need to prove 1 <= V20.1: unknown

It turns out this is implied by the combination of facts from two dominating compares:

***** BB03 [0002]
STMT00001 ( 0x00D[E-] ... 0x00E )
N004 (  5,  5) [000007] -----+-----                           JTRUE     void   $VN.Void
N003 (  3,  3) [000006] J----+-N---                         └──▌  NE        int    $1c1
N001 (  1,  1) [000004] -----+-----                            ├──▌  LCL_VAR   int    V01 arg1         u:1 $c0
N002 (  1,  1) [000005] -----+-----                            └──▌  CNS_INT   int    0 $40

...

***** BB06 [0006]
STMT00016 ( INL02 @ 0x016[E-] ... ??? ) <- INL01 @ ??? <- INLRT @ 0x016[E-]
N004 (  5,  5) [000055] -----+-----                           JTRUE     void   $VN.Void
N003 (  3,  3) [000054] J----+-N---                         └──▌  GE        int    $1c4
N001 (  1,  1) [000034] -----+-----                            ├──▌  LCL_VAR   int    V01 arg1         u:1 $c0
N002 (  1,  1) [000053] -----+-----                            └──▌  CNS_INT   int    0 $40

However, RBO is not able to combine these two facts to prove the 1 <= V20.1. OTOH, range check actually eliminates bounds checks based on the combination of these two facts in this method, so we can likely use assertions or range check directly in addition to RBO to prove it.

<0, $c0 + -1> BetweenBounds <0, [000115]>
$c0 upper bound is:  {InitVal($41)}
Array size is: 1 (minimum)
@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 Dec 2, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 2, 2024
@jakobbotsch jakobbotsch self-assigned this Dec 2, 2024
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Dec 2, 2024
@jakobbotsch jakobbotsch added this to the 10.0.0 milestone Dec 2, 2024
Copy link
Contributor

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

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Dec 3, 2024

OTOH, range check actually eliminates bounds checks based on the combination of these two facts in this method, so we can likely use assertions or range check directly in addition to RBO to prove it.

Actually range check "proves" this in an unsound way, making only use of the != 0 check:

if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) &&
m_pCompiler->vnStore->IsVNCheckedBound(curAssertion->op1.vn))
{
// we have arr.Len != 0, so the length must be atleast one
limit = Limit(Limit::keConstant, 1);
cmpOper = GT_GE;
}

IsVNCheckedBound is not enough on its own to imply non-negativity, so this is not sound. It happens to work because the VNs marked as checked bounds are usually non-negative values, but I do not think that's an IR invariant we want to enforce.

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Dec 3, 2024
IV opts sometimes needs to evaluate some relops symbolically. Before
this PR that evaluation only makes use of RBO. This PR also teaches the
evaluation to make use of range check to evaluate some kinds of relops.

Fix dotnet#110315
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant