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

Avoid creating a result temp for is-expressions #68694

Merged
merged 26 commits into from
Aug 11, 2023
Merged

Avoid creating a result temp for is-expressions #68694

merged 26 commits into from
Aug 11, 2023

Conversation

alrz
Copy link
Member

@alrz alrz commented Jun 20, 2023 •
edited
Loading

Closes #59615
Closes #55334

The idea is that while the expression is still spilled, the expression is no longer spilled by default and the boolean result will be pushed to the stack. The input expression will be evaluated as sequence side effects which will in turn spill if necessary. As a result, codegen is now closer to binary expressions' (but not quite).

The same optimization could be applied to switch expression - looking at conditionals, we will have to merge stack type for that to work. I'll leave it out of this PR for now, I'm new to this so there may be a better way to accomplish the same.

Thanks.

@alrz alrz requested a review from a team as a code owner June 20, 2023 11:37
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 20, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 20, 2023
@AlekseyTs
Copy link
Contributor

@alrz It looks like there are legitimate test failures and Correctness leg errors.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 2), tests are not looked at.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 21, 2023 •
edited
Loading

I am not feeling comfortable with the taken approach. I suggest to look for an alternatives.

@AlekseyTs
Copy link
Contributor

How could I reproduce the bootstrap build locally?

The build script has a-bootstrap switch. Debugging the failure could be tricky though. If I remember correctly I did something like:

  1. Figure out what project cannot be built
  2. Build it under debugger with the new compiler

@alrz
Copy link
Member Author

alrz commented Jun 22, 2023 •
edited
Loading

I am not felling comfortable with the taken approach. I suggest to look for an alternatives.

Short-circuiting is probably best to be done as a rewrite to expression (which doesn't always work because we may have a SwitchDispatch that could turn to an expression in emitter for range checks, which makes me wonder if this optimization should be done in a later pass), for any other case I think Optimizer should be taught to inline the temp when possible. Would that need a new node?

@AlekseyTs
Copy link
Contributor

Would that need a new node?

If we are talking about an optimization as a special pass post regular rewrite (BTW, we already have a pass like that), I hope the optimization could be implemented as looking for a pattern in the bound tree and rewriting portion of the tree. That, generally, shouldn't need a new node. But, who knows, complexity could be so high that having a some sort of hint might help.
In general, introducing new nodes that survive regular rewrite has extra cost and risk, the optimizer might start "failing" on them.

@alrz

This comment was marked as outdated.

Sign in to view
@alrz

This comment was marked as outdated.

Sign in to view
@alrz alrz marked this pull request as draft June 23, 2023 11:00
@alrz alrz force-pushed the dag-lowering branch 2 times, most recently from 5735b22 to 5f13ffe Compare June 26, 2023 16:09
@alrz alrz marked this pull request as ready for review June 28, 2023 22:14
@alrz
Copy link
Member Author

alrz commented Jun 28, 2023 •
edited
Loading

@AlekseyTs Took a different approach to address avoiding spilling temps and including destinations in the same node.
Please take a look when you had a chance. Thanks.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 24)

@alrz
Copy link
Member Author

alrz commented Jul 18, 2023

@AlekseyTs @dotnet/roslyn-compiler for review, thanks.

AlekseyTs
AlekseyTs approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 26)

@jcouv jcouv self-assigned this Jul 18, 2023
@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For the second review

@alrz
Copy link
Member Author

alrz commented Jul 24, 2023

Opened follow-up for switch-expressions: #69194

@alrz
Copy link
Member Author

alrz commented Jul 25, 2023 •
edited
Loading

In #69194, bootstrap build fails where iterator/using are mixed (at least the one cause that I'm aware of). Is there anything special to do to cover this in either PRs?

(Added the details in #69194, but I think it could help to validate this PR as well)

@alrz alrz mentioned this pull request Jul 25, 2023
Draft
@jaredpar jaredpar modified the milestones: 17.8, 17.9 Aug 9, 2023
jcouv
jcouv approved these changes Aug 11, 2023
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 26)

@jcouv
Copy link
Member

jcouv commented Aug 11, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv jcouv enabled auto-merge (squash) August 11, 2023 17:49
@jcouv jcouv merged commit e1031a7 into dotnet:main Aug 11, 2023
@ghost ghost modified the milestones: 17.9, Next Aug 11, 2023
@allisonchou allisonchou mentioned this pull request Aug 16, 2023
Closed
@RikkiGibson RikkiGibson mentioned this pull request Aug 17, 2023
Closed
RikkiGibson added a commit that referenced this pull request Aug 17, 2023
@RikkiGibson
Revert "Avoid creating a result temp for is-expressions (#68694)"
794d6be 
This reverts commit e1031a7.
@RikkiGibson RikkiGibson mentioned this pull request Aug 17, 2023
Merged
RikkiGibson added a commit to CyrusNajmabadi/roslyn that referenced this pull request Aug 17, 2023
@RikkiGibson
Revert "Avoid creating a result temp for is-expressions (dotnet#68694)"
4e45384 
This reverts commit e1031a7.
RikkiGibson added a commit that referenced this pull request Aug 18, 2023
@RikkiGibson
Revert "Avoid creating a result temp for is-expressions (#68694)" (#6…
b4a9c62 
…9582)

This reverts commit e1031a7.
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
@alrz alrz mentioned this pull request Feb 26, 2024
Merged
alrz added a commit to alrz/roslyn that referenced this pull request Mar 7, 2024
@alrz
Reapply "Avoid creating a result temp for is-expressions (dotnet#68694)…
bb65732 
…" (dotnet#69582)

This reverts commit b4a9c62.
AlekseyTs added a commit that referenced this pull request Mar 7, 2024
@AlekseyTs
Revert "Revert "Avoid creating a result temp for is-expressions (#68694…
debc738 
…)" (#6…"

This reverts commit b4a9c62.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.8 P2
5 participants
@alrz @AlekseyTs @jcouv @jaredpar @dibarbet