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

[mono] Don't use recursion in mono_ssa_rename_vars #61677

Merged

Conversation

radekdoulik
Copy link
Member

Implements #60266

Avoid deep recursion in mono_ssa_rename_vars, change the way we traverse
dominated bb's. Instead of using recursion, use stack-like array to
store information about stack history and the traversal.

The performance remains the same (or is slightly better) than before.
Times for the compilation of repro from #57141
(JIT time minimum from 5 runs):

Before:

LLVM output file: './Uno.UI.FluentTheme.dll.bc.tmp'.
JIT time: 4810 ms, Generation time: 2040 ms, Assembly+Link time: 2 ms.

After:

LLVM output file: './Uno.UI.FluentTheme.dll.bc.tmp'.
JIT time: 4781 ms, Generation time: 2017 ms, Assembly+Link time: 2 ms.

Implements dotnet#60266

Avoid deep recursion in mono_ssa_rename_vars, change the way we traverse
dominated bb's. Instead of using recursion, use stack like array to
store information about stack history and the traversal.

The performance remains the same (or is slightly better) than before.
Times for the compilation of repro from dotnet#57141
(JIT time minimum from 5 runs):

Before:

    LLVM output file: './Uno.UI.FluentTheme.dll.bc.tmp'.
    JIT time: 4810 ms, Generation time: 2040 ms, Assembly+Link time: 2 ms.

After:

    LLVM output file: './Uno.UI.FluentTheme.dll.bc.tmp'.
    JIT time: 4781 ms, Generation time: 2017 ms, Assembly+Link time: 2 ms.
Copy link
Member

@lewing lewing 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 ok to me, it would be nice if someone on the runtime side took look.

@lewing
Copy link
Member

lewing commented Nov 18, 2021

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Nov 19, 2021

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marek-safar
Copy link
Contributor

@vargaz @imhameed does this look ok?

Copy link
Contributor

@imhameed imhameed left a comment

Choose a reason for hiding this comment

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

Looks fine. I've added some minor stylistic suggestions. And it would be nice if we had a dedicated dynamic array data structure (instead of needing to roll one from scratch, like is done here), but I don't think this PR should be blocked on that.

src/mono/mono/mini/ssa.c Outdated Show resolved Hide resolved
src/mono/mono/mini/ssa.c Outdated Show resolved Hide resolved
src/mono/mono/mini/ssa.c Outdated Show resolved Hide resolved
src/mono/mono/mini/ssa.c Outdated Show resolved Hide resolved
src/mono/mono/mini/ssa.c Outdated Show resolved Hide resolved
@radekdoulik radekdoulik requested a review from imhameed November 23, 2021 14:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants