Skip to content

Make optVNAssertionPropCurStmtVisitor post-order - #78630

Merged
EgorBo merged 1 commit into
dotnet:mainfrom
EgorBo:post-order-assertprop
Nov 21, 2022
Merged

Make optVNAssertionPropCurStmtVisitor post-order#78630
EgorBo merged 1 commit into
dotnet:mainfrom
EgorBo:post-order-assertprop

Conversation

@EgorBo

@EgorBoEgorBo commented Nov 21, 2022•
edited
Loading

Copy link
Copy Markdown
Member

just to see what spmi thinks about it...

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 21, 2022
@ghostghost assigned EgorBoNov 21, 2022
@ghost

ghost commented Nov 21, 2022

Copy link
Copy Markdown

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

Issue Details

just to see what jit-diffs/pintool think about it...

Author:EgorBo
Assignees:EgorBo
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo
EgorBo marked this pull request as ready for review November 21, 2022 15:28
@EgorBo

EgorBo commented Nov 21, 2022

Copy link
Copy Markdown
MemberAuthor

@SingleAccretion's idea, I was 100% sure it would regress TP 🙂 but it improves it in fact. https://dev.azure.com/dnceng-public/public/_build/results?buildId=89456&view=results

Diffs in libraries collection are not too big that's why I didn't do this change in my previous PR (I though I would regress TP for a small diff).

@EgorBo

EgorBo commented Nov 21, 2022

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch@SingleAccretion PTAL

@SingleAccretion

SingleAccretion commented Nov 21, 2022

Copy link
Copy Markdown
Contributor

If memory is not failing me, this should also fix #57033.

SingleAccretion
SingleAccretion approved these changes Nov 21, 2022
View reviewed changes

@SingleAccretionSingleAccretion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

It is quite curious we see a TP increase. Would be interesting (but not necessarily worthwhile) to find out why that is.

@EgorBo

EgorBo commented Nov 21, 2022

Copy link
Copy Markdown
MemberAuthor

If memory is not failing me, this should also fix #57033.

Yeah, it folds the whole thing to return 4 🙂 awesome, thanks!

@jakobbotsch

jakobbotsch commented Nov 21, 2022

Copy link
Copy Markdown
Member

Seems like there's some more low hanging fruit here TP wise by using a GenTreeVisitor (not necessarily for this PR)

@EgorBo
EgorBo merged commit 1ab2c4a into dotnet:mainNov 21, 2022
@EgorBo

EgorBo commented Nov 21, 2022

Copy link
Copy Markdown
MemberAuthor

SPMI failure:

Failed to download unix-arm-3/1/superpmi_Linux_arm_3.log blob from results container:

Presumably collection failed on that one yesterday and we need to kick it off again

@EgorBo
EgorBo deleted the post-order-assertprop branch November 21, 2022 16:09
@EgorBoEgorBo mentioned this pull request Nov 21, 2022
Closed
EgorBo added a commit to EgorBo/runtime-1 that referenced this pull request Nov 22, 2022
@EgorBo
Revert "Make optVNAssertionPropCurStmtVisitor post-order (dotnet#78630)"
232113d
This reverts commit 1ab2c4a.
This was referenced Nov 22, 2022
Merged
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 21, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes
+1 more reviewer
@SingleAccretionSingleAccretionSingleAccretion approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR 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.

3 participants

@EgorBo@SingleAccretion@jakobbotsch