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: revise how we track if nodes have been morphed #110787

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

AndyAyersMS
Copy link
Member

Allow remorphing. Require that all nodes be morphed. Ensure that no node is morphed more than a handful of times.

Fixes #6218, #56962, #107542

Allow remorphing. Require that all nodes be morphed. Ensure that no node
is morphed more than a handful of times.

Fixes dotnet#6218, dotnet#56962, dotnet#107542
@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 Dec 17, 2024
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Dec 17, 2024

Based on #110658 (comment)

There are two main concerns with morph:

  1. Every node should be morphed at least once, both because of required changes morph will make and because we are in the middle of propagating assertions. We were not morphing quite a few nodes created in post-order, though they mostly fall into two camps: (a) trivial leaf or near-leaf cases where there is nothing for morph to do (either any opportunity for assertion prop already happened or is not applicable); or (b) assignments to newly introduced temps that will have no "remote" uses (so we miss out on assertion gen/kill, but again, largely to no effect, as there are no redefs and all uses are local and can't be optimized).
  2. We should try avoiding morphing nodes repeatedly. Also tricky to avoid in post-order, possibly (but unlikely) wrong, and in general is just inefficient.

With this change we enforce that all nodes must be "morphed" though this is weaker than ensuring that all nodes have done the proper amount of morphing. Also we attempt to count how many times each node goes though the fully general morphing process and assert if this happens too often (we may be missing cases where an "enlightened" re-morph bypasses the upper levels of morphs' general dispatch).

There is a possible opportunity for assertion gen/kill for return value canonicalization; I have left that for a follow-up PR. I do not think adding assertion gen/kill for the other post-order assignments will yield any interesting diffs but we might want to insist on it anyways.

The unbox transformation introduces new temp assignments in a remote tree, which morph's assertion prop is unable to model. This is fairly stylized and "works" since the box creation must dominate the box use, and there are no other uses of the introduced temps.

No diffs, no impact in release.

With this change the main burden going forward is on new/modified postorder expansions (and the utility methods involved), generally marking things as morphed where needed, or (rarely) allowing a full remorph.

@jakobbotsch PTAL
cc @dotnet/jit-contrib

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.

Seems like an improvement on the current state of things, but I think it would be ideal in the long term if we could ensure all nodes are morphed exactly once, and go back to an even stricter version of the checking we had before. Clearly that requires some non-trivial refactoring of morph, though, although seems like it is long overdue at this point.

@AndyAyersMS
Copy link
Member Author

Likely will need some ABI or ISA specific fixes here and there.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr jitstress

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr jitstress

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

Remove GTF_DEBUG_NODE_MORPHED.
2 participants