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

JIT: fix issue with assert seen in OSR+PGO tests #69067

Merged
merged 5 commits into from
May 11, 2022
Merged

JIT: fix issue with assert seen in OSR+PGO tests #69067

merged 5 commits into from
May 11, 2022

Conversation

AndyAyersMS
Copy link
Member

IsIconHandle only works if we know the tree is an integer.

Closes #69032.

@AndyAyersMS
JIT: fix issue with assert seen in OSR+PGO tests
13f7361 
`IsIconHandle` only works if we know the tree is an integer.

Closes dotnet#69032.
@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 May 9, 2022
@ghost ghost assigned AndyAyersMS May 9, 2022
@ghost
Copy link

ghost commented May 9, 2022

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

Issue Details

IsIconHandle only works if we know the tree is an integer.

Closes #69032.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

jakobbotsch
jakobbotsch approved these changes May 9, 2022
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Is there any reason we couldn't just move these functions into GenTreeIntCon?

SingleAccretion
SingleAccretion reviewed May 9, 2022
View reviewed changes
src/coreclr/jit/gentree.cpp Outdated
@@ -16487,7 +16487,7 @@ bool GenTree::IsBlockProfileUpdate()

GenTree* const addr = lhs->AsIndir()->Addr();

return addr->IsIconHandle(GTF_ICON_BBC_PTR);
return addr->IsIntegralConst() && addr->IsIconHandle(GTF_ICON_BBC_PTR);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the more technically correct test would be IsCnsIntOrI as IsIntegralConst will be true for CNS_LNG as well.

@AndyAyersMS
Copy link
Member Author

Is there any reason we couldn't just move these functions into GenTreeIntCon?

You mean the IsIconHandle overloads?

@AndyAyersMS
Copy link
Member Author

Seems like I should also add a test.

@jakobbotsch
Copy link
Member

You mean the IsIconHandle overloads?

Yes (for all the functions around here that assert gtOper == GT_CNS_INT). Assuming we do not end up with other nodes with the GT_CNS_INT oper.

Either that or I think IsIconHandle and co. should check for the correct oper on their own. Currently these functions end up being sort of dynamically typed for seemingly no good reason (probably a vestige from before the node inheritance hierarchy existed).

@AndyAyersMS
Copy link
Member Author

Ok, let me look into moving those -- depending on what I see for callers I'll either generalize them to work for any tree or move them down off the base class.

@AndyAyersMS
Copy link
Member Author

Ok, let me look into moving those -- depending on what I see for callers I'll either generalize them to work for any tree or move them down off the base class.

There are about 100 uses so seems like generalizing them to work for any tree would be the least disruptive.

@AndyAyersMS
Copy link
Member Author

Ok, I made IsIconHandle work for all trees. The get/clear methods I left alone.

There is also quite a bit of code that checks for non-handle ints: if (op1->IsCnsIntOrI() && !op1->IsIconHandle()) which could arguably be encapsulated with yet another predicate helper, but I also left those as is for now.

@AndyAyersMS
Copy link
Member Author

Libraries failure looks like #68443.
Timeout on arm64 should be addressed by #69129.

@AndyAyersMS AndyAyersMS merged commit 10a4986 into dotnet:main May 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SingleAccretion SingleAccretion SingleAccretion left review comments

@BruceForstall BruceForstall BruceForstall approved these changes

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@AndyAyersMS AndyAyersMS

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.

JIT: libraries PGO assert during Find Loops
4 participants
@AndyAyersMS @jakobbotsch @BruceForstall @SingleAccretion