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] Don't emit a redundant move in lazy fetch trampolines on AMD64. #5346

Merged
merged 1 commit into from
Aug 18, 2017

Conversation

vkargov
Copy link
Contributor

@vkargov vkargov commented Aug 9, 2017

Hello,

This simple patch is meant to remove meaningless movq %rdi, %rdi instructions generated in rgctx fetch trampolines.
For instance, jitting csc.exe would cause 37 such moves to appear.

A more general approach would be making *_mov_reg_reg skip on move construction if it sees the source and the destination are the same, but I'm cautious that some of the existing code may be somehow relying on the expectation that a dummy instruction will always be built. (it doesn't appear so, though)

@lewurm
Copy link
Contributor

lewurm commented Aug 9, 2017

how about you are trying your suggested general approach first? submit a PR and let us see what breaks 🙂

@vargaz
Copy link
Contributor

vargaz commented Aug 9, 2017

It would be better to just use an if () instead of an #ifdef, it looks better, and the compiler will optimize it away the same.

@vargaz
Copy link
Contributor

vargaz commented Aug 9, 2017

Codegen macros should generate code, they should not do optimization.

@vkargov
Copy link
Contributor Author

vkargov commented Aug 17, 2017

Thanks for the feedback, I've replaced the macro with an if as suggested.

@lewurm
I experimented with that general approach prior to opening this PR and it did not break in an obvious way, i.e. it compiled and all tests passed. However, I'm not completely certain it wouldn't break on some edge case.
Either way, I found out that all redundant moves are generated by that one function this patch is meant to fix, so for all practical purposes it should get rid of the only known cause.
Sorry, I probably should have been more verbose on this from the beginning.

@lewurm
Copy link
Contributor

lewurm commented Aug 17, 2017

@monojenkins rebase

@monojenkins
Copy link
Contributor

cannot rebase:

  • "Linux i386" state is "failure"
  • "Linux x64" state is "failure"
  • "OS X i386" state is "failure"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "failure"

@monojenkins
Copy link
Contributor

cannot rebase:

  • "Linux i386" state is "failure"
  • "Linux x64" state is "success"
  • "OS X i386" state is "failure"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "success"

@monojenkins
Copy link
Contributor

cannot rebase:

  • "Linux i386" state is "success"
  • "Linux x64" state is "success"
  • "OS X i386" state is "failure"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "success"

@vargaz vargaz merged commit 25436ac into mono:master Aug 18, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants