982131 - OdinMonkey: GenerateFFIIonExit doesn't need to save non-volatiles

Status: RESOLVED DUPLICATE of bug 898963

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

I just noticed that GenerateFFIIonExit saves non-volatile registers.  Since Odin uses the same calling convention as Ion, where the callee is assumed to clobber everything, this is unnecessary.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch

Seemed too easy, but all tests pass (and benchmarks too) so I assume it's good.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8390015 [details] [diff] [review]
Patch

Wrong flag set.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8390015 [details] [diff] [review]
Patch

Thanks!

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

I've ran a try build to be sure, and I am running into issues only with the ARM simulator in debug mode.

Try all platforms, d/o (with another patch)
https://tbpl.mozilla.org/?tree=Try&rev=fb9588c5d6f4

Try ARM simulator (only this patch):
https://tbpl.mozilla.org/?tree=Try&rev=da6ef962b705

This patch only removes the push of non-volatile registers when calling from Odin into Ion.
Jandem, do you have any idea of what's going on here? Do you see a better solution than ifdef'ing the removed guilty lines?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> Jandem, do you have any idea of what's going on here? Do you see a better
> solution than ifdef'ing the removed guilty lines?

Can you reproduce it locally? There's a bug somewhere that may also affect real ARM hardware (we don't run jit-tests on ARM on tbpl) and we should know what it is (ifdef'ing is sweeping it under the rug).

Comment 6

[Douglas Crosher [:dougc]]

Odin code has some pinned registers on the x64 and ARM that will need to be restored.  The patch in bug 898963 has been updated to also avoid unnecessarily saving registers, so perhaps this could be marked as a dup or visa versa.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Douglas Crosher [:dougc] from comment #6)
> Odin code has some pinned registers on the x64 and ARM that will need to be
> restored.  The patch in bug 898963 has been updated to also avoid
> unnecessarily saving registers, so perhaps this could be marked as a dup or
> visa versa.

Sure, patch in bug 898963 seems indeed more complete.