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

Don't update side-effects in a statement that doesn't exist #113351

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 10, 2025

Fixes #113334

There were two issues:

  1. fgMorphBlock could fold the entire upper bound check into no-op meaning there are no statements to update side-effects for
  2. (unrelated to the bug) - we shouldn't mix different bounds checks nodes with different throw kind

IR for lower-bound block and then upper-bound block:

------------ BB04 [0003] [012..012) -> BB06(0),BB05(1) (cond), preds={BB01} succs={BB05,BB06}

***** BB04 [0003]
STMT00006 ( 0x012[E-] ... ??? )
N004 (  5,  5) [000136] -----------                         *  JTRUE     void
N003 (  3,  3) [000135] J----------                         \--*  GT        int
N001 (  1,  1) [000133] -----------                            +--*  CNS_INT   int    0
N002 (  1,  1) [000134] -----------                            \--*  CNS_INT   int    0 $40

------------ BB05 [0004] [012..012) -> BB02(1),BB06(0) (cond), preds={BB04} succs={BB06,BB02}

***** BB05 [0004]
STMT00007 ( 0x012[E-] ... ??? )
N004 (  5,  5) [000139] -----------                         *  JTRUE     void
N003 (  3,  3) [000138] J----------                         \--*  LT        int
N001 (  1,  1) [000137] -----------                            +--*  CNS_INT   int    0
N002 (  1,  1) [000131] -----------                            \--*  CNS_INT   int    1 $43

@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 Mar 10, 2025
@jakobbotsch
Copy link
Member

I wonder -- do we gain a lot from calling morph over removing the bounds checks manually?

@EgorBo
Copy link
Member Author

EgorBo commented Mar 11, 2025

I wonder -- do we gain a lot from calling morph over removing the bounds checks manually?

@jakobbotsch Yes, it's needed to handle constant indices - there are no phases to fold JTRUE(1 > 0) after it (for upper bound check). I can fold it by hands if we want to avoid morph.. that will just be more verbose.

I just didn't expect the lower bound check can be also foldable - seems to happen only around bounds checks with SIMD.

@EgorBo EgorBo marked this pull request as ready for review March 11, 2025 13:23
@Copilot Copilot bot review requested due to automatic review settings March 11, 2025 13:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses a bug where side-effects were not updated due to an entirely folded upper-bound check and also separates bounds check nodes by throw kind.

  • Fix the issue in fgMorphBlock where no statements are updated when the upper-bound check is a no-op.
  • Prevent mixing of different bounds check nodes with different throw kinds.
  • Introduce a regression test to verify the fix.

@EgorBo EgorBo merged commit bf6cfef into dotnet:main Mar 11, 2025
120 checks passed
@EgorBo EgorBo deleted the fix-clonned-rngchk branch March 11, 2025 21:54
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2025
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
Development

Successfully merging this pull request may close these issues.

JIT: Assertion failed 'use != nullptr' during 'Clone blocks with range checks'
2 participants