Skip to content

JIT: Use bbPostorderNum to track ordinals in 3-opt #111464

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
Merged
amanasifkhalid merged 1 commit into from
Jan 16, 2025
Merged

JIT: Use bbPostorderNum to track ordinals in 3-opt #111464

amanasifkhalid merged 1 commit into from
Jan 16, 2025

Conversation

@amanasifkhalid
Copy link
Contributor

@amanasifkhalid amanasifkhalid commented Jan 15, 2025

Follow-up to #111365. Instead of allocating a separate ordinal array, track each block's ordinal by overwriting its bbPostorderNum. We can check if a given block is in the candidate set of blocks being reordered if its ordinal is less than the number of hot blocks, and if the block at the given ordinal in blockOrder matches the block in question (i.e. the same implementation as FlowGraphDfsTree::Contains.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 15, 2025
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Jan 15, 2025

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

@amanasifkhalid
Copy link
Contributor Author

amanasifkhalid commented Jan 15, 2025

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. No diffs. Thanks!

AndyAyersMS
AndyAyersMS reviewed Jan 15, 2025
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Should we take precautions so that nobody looks at these numbers later hoping they make sense?

Probably invalidating the DFS tree is sufficient, if we're not already doing that?

@amanasifkhalid
Copy link
Contributor Author

amanasifkhalid commented Jan 16, 2025

Probably invalidating the DFS tree is sufficient, if we're not already doing that?

We already invalidate the DFS tree after running 3-opt, though I suppose I could move that earlier (i.e. right after we set the initial ordinal numbers before reordering) to be safe?

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Jan 16, 2025

Probably invalidating the DFS tree is sufficient, if we're not already doing that?

We already invalidate the DFS tree after running 3-opt, though I suppose I could move that earlier (i.e. right after we set the initial ordinal numbers before reordering) to be safe?

Afterwards is fine.

@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 16, 2025 •
edited
Loading

Note that the post phase check in fgDebugCheckFlowGraphAnnotations already validates that the postorder numbers are correct if the DFS tree is set, so it shouldn't be possible to leave it stale.

@amanasifkhalid amanasifkhalid merged commit 73a9330 into dotnet:main Jan 16, 2025
109 checks passed
@amanasifkhalid amanasifkhalid deleted the 3-opt-ordinals branch January 16, 2025 14:38
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees

@amanasifkhalid amanasifkhalid

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.

3 participants

@amanasifkhalid @AndyAyersMS @jakobbotsch