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

Allow the inliner to substitute for small arguments #69068

Merged
merged 4 commits into from
May 17, 2022

Conversation

jakobbotsch
Copy link
Member

Since we no longer have contextual nodes to indicate small arguments
this should be safe to do. For register arguments it does not matter and
for stack arguments it is handled in codegen (for macOS arm64).

Since we no longer have contextual nodes to indicate small arguments,
this should be safe to do. For register arguments it does not matter and
for stack arguments it is handled in codegen (for macOS arm64).
@ghost ghost assigned jakobbotsch May 9, 2022
@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 May 9, 2022
@ghost
Copy link

ghost commented May 9, 2022

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

Issue Details

Since we no longer have contextual nodes to indicate small arguments
this should be safe to do. For register arguments it does not matter and
for stack arguments it is handled in codegen (for macOS arm64).

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

With small args we could end up inserting a cast between the arg node
and a GT_RET_EXPR. We would not handle substituting this GT_RET_EXPR
correctly. This reverts some more of the changes from 3e65d68 to make
the scenario work again, by skipping over GT_RET_EXPR before creating
the inlinee arg nodes.
@jakobbotsch
Copy link
Member Author

jakobbotsch commented May 10, 2022

This is waiting on #69117

@jakobbotsch
Copy link
Member Author

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member Author

Failures are #69349, #68690, #69350, #69232, #69154, #68513

@jakobbotsch
Copy link
Member Author

jakobbotsch commented May 16, 2022

cc @dotnet/jit-contrib PTAL @AndyAyersMS
I need to spot check the diffs, but this is generally an improvement and just undoes the limitation added in #43130.

@jakobbotsch jakobbotsch requested a review from AndyAyersMS May 16, 2022 09:37
@jakobbotsch
Copy link
Member Author

Spot-checked some of the regressions, they fell in the following categories:

  1. Different (worse) register allocation, with some additional spills
  2. Different alignment
  3. We create more locals now (that we later may substitute if they are single-used) -- more locals lead to different inlining decisions (Inliner uses capacity of locals table instead of count #69280 related)
  4. Fewer statements in BBs mean we sometimes duplicate more conditional jumps (fgOptimizeUncondBranchToSimpleCond) -- this leads to more code but a shorter critical path

@jakobbotsch jakobbotsch merged commit f276dc7 into dotnet:main May 17, 2022
@jakobbotsch jakobbotsch deleted the inliner-substitution-small-types branch May 17, 2022 10:19
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2022
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