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

JIT: fix fgDfsReversePostorder #82950

Merged
merged 1 commit into from
Mar 3, 2023
Merged

JIT: fix fgDfsReversePostorder #82950

merged 1 commit into from
Mar 3, 2023

Conversation

AndyAyersMS
Copy link
Member

This method was not doing a proper depth first search, so the reverse postorder it created was flawed.

This method was not doing a proper depth first search, so the reverse postorder
it created was flawed.
@ghost ghost assigned AndyAyersMS Mar 3, 2023
@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 3, 2023
@ghost
Copy link

ghost commented Mar 3, 2023

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

Issue Details

This method was not doing a proper depth first search, so the reverse postorder it created was flawed.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

See #82926 (comment) for context.

No diffs, which is a bit surprising.

@AndyAyersMS AndyAyersMS requested a review from jakobbotsch March 3, 2023 18:14
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.

LGTM.

Is this equivalent to the existing code if we did the visited check/update when we actually pop the stack instead of before pushing on the stack?

@AndyAyersMS
Copy link
Member Author

Is this equivalent to the existing code if we did the visited check/update when we actually pop the stack instead of before pushing on the stack?

I originally wondered that too, think that it would be messed up by cycles. EG a self cycle would repeatedly push / pop / push / pop. A second bit vector might fix this -- say tracking when a block is first popped and not pushing after that.

@AndyAyersMS AndyAyersMS merged commit f59a3f5 into dotnet:main Mar 3, 2023
dubiousconst282 added a commit to dubiousconst282/DistIL that referenced this pull request Mar 8, 2023
The old algorithm was recently introduced in 26b6d0e. Although it is simpler, it
may produce incorrect post-orderings, which may become problematic in the future.

Coincidentally it was exactly the same algorithm used in RyuJIT, and the bug
is so subtle that they just recently discovered it :') - dotnet/runtime#82950

This also adds a few micro opts in SuccIterator for perf and mem overhead.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants