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

Call Enter/LeaveRegion APIs for binary expressions #76412

Merged
merged 8 commits into from
Dec 17, 2024

Conversation

RikkiGibson
Copy link
Contributor

Closes #38087

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 13, 2024
@@ -2553,6 +2553,7 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node)
protected virtual void VisitBinaryOperatorChildren(ArrayBuilder<BoundBinaryOperator> stack)
{
var binary = stack.Pop();
EnterRegionIfNeeded(binary);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't call 'VisitAlways' on the binary nodes themselves to avoid recursion. Therefore we need to take care to call Enter/LeaveRegion APIs appropriately when we start/finish with those nodes.

See also #74317 for a non-recursive visitor which makes similar calls.

@CyrusNajmabadi
Copy link
Member

Thanks. I can also hellp with adding an ExtractMethod test if you need it.

@@ -2541,6 +2541,7 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node)
do
{
stack.Push(binary);
EnterRegionIfNeeded(binary);
Copy link
Contributor Author

@RikkiGibson RikkiGibson Dec 13, 2024

Choose a reason for hiding this comment

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

We need to take care that Enter is called for all the binary ops, from outermost to innermost, and then Leave is called from innermost to outermost. Most convenient way to do this is to call Enter right after Push, then Leave right before "Pop or exit".

@RikkiGibson RikkiGibson marked this pull request as ready for review December 13, 2024 20:44
@RikkiGibson RikkiGibson requested a review from a team as a code owner December 13, 2024 20:44
@RikkiGibson
Copy link
Contributor Author

@CyrusNajmabadi If you have some time feel free to push an ExtractMethod test to this PR.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner December 13, 2024 21:02
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler for review of a small fix

@AlekseyTs
Copy link
Contributor

Does VB have the same issue?

@RikkiGibson
Copy link
Contributor Author

VB implementation appears to be accounting for this when visiting binary expressions.

' As we emulate visiting, enter the region if needed.
' We shouldn't do this for the top most node,
' VisitBinaryOperator caller such as VisitRvalue(...) does this.
If Me._regionPlace = RegionPlace.Before Then
If stack.Count > 0 Then
' Skipping the first node
Debug.Assert(stack(0) Is node)
For i As Integer = 1 To stack.Count - 1
If stack(i) Is Me._firstInRegion Then
Me.EnterRegion()
GoTo EnteredRegion
End If
Next
' Note, the last binary operator is not pushed to the stack, it is stored in [binary].
Debug.Assert(binary IsNot node)
If binary Is Me._firstInRegion Then
Me.EnterRegion()
End If
EnteredRegion:
Else
Debug.Assert(binary Is node)
End If
End If

But, at a glance, I didn't find a test in RegionAnalysisTests.vb which is doing a dataflow analysis on a nested binary operator.

@AlekseyTs
Copy link
Contributor

Consider adding a test

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 7)

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 8)

@RikkiGibson RikkiGibson requested a review from a team December 16, 2024 20:01
@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Dec 16, 2024
@RikkiGibson RikkiGibson merged commit b2b9e32 into dotnet:main Dec 17, 2024
28 checks passed
@RikkiGibson RikkiGibson deleted the binary-region branch December 17, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract Method on part of an expression generates invalid code
6 participants