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.

Sign up for GitHub

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

Jump to bottom

Refactoring in AssertrionDesc #113440

Merged
merged 2 commits into from
Mar 26, 2025
Merged

Refactoring in AssertrionDesc #113440

merged 2 commits into from
Mar 26, 2025

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 12, 2025 •
edited
Loading

This PR does a small refactoring in assertprop.cpp:

  • SsaVar (with its ssaNum) is removed from AssertrionDesc. We used to use it for O2K_LCLVAR_COPY in Global AP assertions. Now that O2K_LCLVAR_COPY is Local AP only, we don't need to record the SSA num. In Global AP we rely purely on VNs.
  • SUBRANGE assertions are now Local AP only (since they don't generate diffs for Global AP).

No diffs

@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 12, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@EgorBo EgorBo marked this pull request as ready for review March 13, 2025 06:37
@Copilot Copilot bot review requested due to automatic review settings March 13, 2025 06:37
Copilot
Copilot AI reviewed Mar 13, 2025
View reviewed changes
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.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

@EgorBo
Copy link
Member Author

EgorBo commented Mar 13, 2025

/azp list

@azure-pipelines Azure Pipelines

This comment was marked as resolved.

Sign in to view
@EgorBo
Copy link
Member Author

EgorBo commented Mar 13, 2025

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

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Mar 13, 2025 •
edited
Loading

PTAL @AndyAyersMS @dotnet/jit-contrib (see desc)

Outerloop failures aren't related

@EgorBo EgorBo requested a review from AndyAyersMS March 13, 2025 21:30
@EgorBo EgorBo changed the title Refactor assertprop a bit Refactoring in AssertrionDesc Mar 14, 2025
AndyAyersMS
AndyAyersMS reviewed Mar 14, 2025
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Are the diffs mainly from the extra isUnsigned handling?

I always prefer isolating refactoring from changing functionality so there is more confidence the refactoring was faithful. But will leave it up to you to decide if that's more trouble here than it is worth.

I think moving assertion creation closer to the point where we have the facts makes sense, I never liked how optCreateAssertionn seemingly has to re-discover what kind of assertion we're supposed to be making.

@EgorBo
Copy link
Member Author

EgorBo commented Mar 14, 2025 •
edited
Loading

Are the diffs mainly from the extra isUnsigned handling?

I always prefer isolating refactoring from changing functionality so there is more confidence the refactoring was faithful. But will leave it up to you to decide if that's more trouble here than it is worth.

Unfortunately, I can't make it zero-diff, but let me file a separate PR where I'll do the changes with diffs and keep this one zero-diff.

I think moving assertion creation closer to the point where we have the facts makes sense, I never liked how optCreateAssertionn seemingly has to re-discover what kind of assertion we're supposed to be making.

Yep, this is one of the goals, not yet there 🙂

@EgorBo EgorBo mentioned this pull request Mar 15, 2025
Merged
EgorBo added a commit that referenced this pull request Mar 19, 2025
@EgorBo
JIT: Changes from #113440 with diffs (#113559)
6a23f1c
@EgorBo EgorBo force-pushed the refactor-assertprop branch from 246711b to a60403d Compare March 22, 2025 00:49
@EgorBo
Copy link
Member Author

EgorBo commented Mar 22, 2025

@AndyAyersMS could you please take a look again? I've removed some changes and made it 0 diff. Will refactor the creation of assert separately.

@EgorBo EgorBo requested a review from AndyAyersMS March 22, 2025 16:59
@EgorBo
Copy link
Member Author

EgorBo commented Mar 26, 2025

ping @AndyAyersMS

AndyAyersMS
AndyAyersMS approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS 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 curious if we're spuriously calling into fgAssertionGen somewhere.

src/coreclr/jit/morph.cpp
@@ -11839,21 +11839,21 @@ void Compiler::fgAssertionGen(GenTree* tree)
//
auto addImpliedAssertions = [=](AssertionIndex index, ASSERT_TP& assertions) {
AssertionDsc* const assertion = optGetAssertion(index);
if ((assertion->assertionKind == OAK_EQUAL) && (assertion->op1.kind == O1K_LCLVAR) &&
if (optLocalAssertionProp && (assertion->assertionKind == OAK_EQUAL) && (assertion->op1.kind == O1K_LCLVAR) &&
Copy link
Member

Choose a reason for hiding this comment

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

How are we calling this outside of global morph?

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndyAyersMS we don't, I forgot to remove this redundant check when I was investigating a bug 🙂 I'll push a commit to remove it in #113920 clean up PR to avoid spinning CI again

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed: 97449fb

@EgorBo EgorBo merged commit 0a827c4 into dotnet:main Mar 26, 2025
108 checks passed
@EgorBo EgorBo deleted the refactor-assertprop branch March 26, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees

@EgorBo EgorBo

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

Successfully merging this pull request may close these issues.
2 participants
@EgorBo @AndyAyersMS