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: Add pred block iterator that tolerates pred list modifications #99466

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

amanasifkhalid
Copy link
Member

@amanasifkhalid amanasifkhalid commented Mar 8, 2024

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.

I haven't found the need to update the pred edge iterator to allow updates too, but I imagine it would look the same as the pred block iterator.

fgTailMergeThrows looks a bit awkward right now. After this change is merged, I plan to add a new method similar to fgRedirectTargetEdge for BBJ_COND blocks, and leverage it in fgReplaceJumpTarget. Since that new method will preserve edge weights, we won't lose anything in fgTailMergeThrows by just calling fgReplaceJumpTarget. I think that will be a good opportunity to clean up fgTailMergeThrows by getting rid of all its helpers for updating edges.

cc @dotnet/jit-contrib, @AndyAyersMS PTAL.

@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 8, 2024
Copy link
Contributor

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

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.

I like this direction, but would prefer that the nature of the iterator be obvious from its name..

Maybe something like:

class PredBlockList
{
public:

   static PredBlockList PredBlocks() const
   {
      return PredBlockList(bbPreds, /*allowEdits*/ false);
   }

   static PredBlockList PredBlocksEditing() const
   {
      return PredBlockList(bbPreds, /*allowEdits*/ true);
   }

private:

   PredBlockList(FlowEdge* list, bool allowEdits) {...};
};

// If allowEdits=true, the user can modify the predecessor list while traversing it,
// so (next == m_next) may not be true. Since we cached the next pointer in m_next, this won't break iteration.
assert((next == m_next) || allowEdits);
updateNextPointer = true;
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be modifying live state under debug, which I think we should avoid.

@amanasifkhalid
Copy link
Member Author

@AndyAyersMS I separated out the allowEdits=true path in operator++. It has a bit of duplicated code now, but I think it makes it clearer that m_next is always updated for editing iterators, but only updated for non-editing iterators in Debug, so there shouldn't be any newly-introduced cases where we update state in Debug only. I also added PredBlocksEditing, but kept the templated pattern with the hope that we can avoid some branches in the iterator logic -- are you ok with this approach?

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.

This looks great.

@amanasifkhalid
Copy link
Member Author

Thanks for the reviews! I should note that this change has small diffs from slight changes in edge weights.

@amanasifkhalid amanasifkhalid merged commit 3eb8c7f into dotnet:main Mar 11, 2024
127 of 129 checks passed
@amanasifkhalid amanasifkhalid deleted the pred-iterator branch March 11, 2024 02:21
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
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