-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: Add helper method for updating bbTargetEdge #99362
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsPart of #93020. Adds Note that I didn't update
|
cc @dotnet/jit-contrib, @AndyAyersMS PTAL. This change has some small diffs. I was expecting this to be a no-diff change, but it looks like there are a few |
We had this sort of setup in Phoenix, there were "Editing" iterators that allowed modification with explicit resume points, and regular iterators that did a limited amount of checking in debug to try and verify the collections weren't modified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment typo -- feel free to fix later.
I'm leaning towards this approach; I'll try this out in a follow-up PR. |
Co-authored-by: Andy Ayers <[email protected]>
…99466) Follow-up to #99362. fgRedirectTargetEdge modifies the predecessor lists of the old and new successor blocks, so if we want to be able to use it in fgReplaceJumpTarget, we need a pred block iterator that is resilient to these changes, as we frequently call the latter method while iterating predecessors.
Follow-up to #99362. Adds helper methods for redirecting BBJ_COND blocks' true/false targets to new successors while reusing the existing flow edge.
Part of #93020. Adds
fgRedirectTargetEdge
, which updates bbTargetEdge's block target while maintaining the rest of the edge's state to simplify edge profile maintenance (and avoid additional allocations). My goal is to eventually have some generalized version of this that can replace any block's successor edges without having to consider the block's kind, such as by updating the edge via aFlowEdge**
. I tried this locally, and there are some quirks around duplicate edges that complicate this approach, and it might make sense to have a fast path for updating blocks with only one successor anyway, so I'm starting here.Note that I didn't update
fgReplaceJumpTarget
to use this yet, as the method is frequently called while iterating a block's predecessors, andfgRedirectTargetEdge
modifiesbbPreds
, breaking the iterator. This can be easily fixed through multiple approaches, though I'm not sure which one is preferable:fgReplaceJumpTarget
, though this isn't as elegant.m_next
pointer in pred iterators in Debug builds for consistency checking. We can make this available all the time, and then update the iterator to it instead of usingm_pred->getNextPredEdge
, as that will traverse the pred list of the new target. This will impact TP, so perhaps this should be tried in a separate PR.