OdinMonkey: cleanups and tests post bug 1022142

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Some of the cleanups and tests we discussed during bug 1022142 debugging.
(Assignee)

Comment 1

4 years ago
Created attachment 8439228 [details] [diff] [review]
1. Kill NativeFrameSize

As discussed, since bug 860736, AlignmentAtAsmJSPrologue and NativeFrameSize are equivalent and used for the same purpose, so we can get rid of one of them.
Attachment #8439228 - Flags: review?(luke)
(Assignee)

Comment 2

4 years ago
Created attachment 8439229 [details] [diff] [review]
2. Moar asm.js => ion testing

More tests for the asm.js => ion path:
- calls with all arguments on registers
- (your test in the previous bug was already testing calls with arguments getting on the stack)
- calls going through the throwLabel path
- OOL convert for int32
- OOL convert for doubles
Any other ideas of missing tests?
Attachment #8439229 - Flags: review?(luke)

Comment 3

4 years ago
Actually, on further thought, I realized that "AlignmentAtPrologue" isn't really the best name... before this patch, it was only used in alignment calculations, but that's not really what it represents.  What it really represents is: how many bytes were pushed before those recorded in masm.framePushed?  It's only when you combine this number with the assumption that calls happen with aligned stack pointers that you can reason about alignment.  So how about renaming both AsmJSAlignmentAtCall/NativeFrameSize to AsmJSBytesPushedByCall (with a comment in Assembler-arm/mips.h that we are pretending that lr/ra is pushed by the call and a comment in CodeGenerator(ARM,MIPS)::generateAsmJSPrologue referring to this comment).

(I just realized that, in theory, instead of changing ARM to match x86, we could have done the reverse by setting masm.setFramePushed(sizeof(void*)) for x86/x64, removing AsmJSBytesPushedByCall.  I expect this would have had a separate set of problems.)

Comment 4

4 years ago
Comment on attachment 8439229 [details] [diff] [review]
2. Moar asm.js => ion testing

Nice!
Attachment #8439229 - Flags: review?(luke) → review+

Comment 5

4 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Any other ideas of missing tests?

Nope, that'd good.

Btw, adding to comment 3, you could also replace all the masm.push(lr) comments in AsmJS.cpp with ones that just reference AsmJSBytesPushedByCall.
(Assignee)

Comment 6

4 years ago
Created attachment 8439374 [details] [diff] [review]
1. Kill NativeFrameSize and rename AlignmentAtAsmJSPrologue into AsmJSSizeOfRetAddr

As discussed on IRC, let's use the name AsmJSSizeOfRetAddr, to keep it simple. Also modified the comments as indicated.
Attachment #8439228 - Attachment is obsolete: true
Attachment #8439228 - Flags: review?(luke)
Attachment #8439374 - Flags: review?(luke)

Comment 7

4 years ago
Comment on attachment 8439374 [details] [diff] [review]
1. Kill NativeFrameSize and rename AlignmentAtAsmJSPrologue into AsmJSSizeOfRetAddr

Review of attachment 8439374 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jit/AsmJS.cpp
@@ +6109,5 @@
>      // PushRegsInMask(NonVolatileRegs).
>      masm.setFramePushed(0);
>  
>  #if defined(JS_CODEGEN_ARM)
> +    // See AsmJSSizeOfRetAddr comment in Assembler-arm.h.

Can you move this comment right above the #ifdef and change "arm" to "*"?

@@ +6368,5 @@
>      m.setInterpExitOffset(exitIndex);
>      masm.setFramePushed(0);
>  
>  #if defined(JS_CODEGEN_ARM)
> +    // See AsmJSSizeOfRetAddr comment in Assembler-arm.h.

ditto

@@ +6544,5 @@
>  
>  #if defined(JS_CODEGEN_X64)
>      masm.Push(HeapReg);
>  #elif defined(JS_CODEGEN_ARM)
> +    // See AsmJSSizeOfRetAddr comment in Assembler-arm.h.

ditto

::: js/src/jit/arm/Assembler-arm.h
@@ +140,5 @@
>  static const bool StackKeptAligned = true;
> +
> +// In AsmJS, we force the return address to be pushed before a call, so as to
> +// harmonize the ABI between all platforms. This has to be taken into account
> +// when computing stack offsets.

Hm, "pushed before a call" is a bit confusing (as the push happens after the call instruction).  How about:

  As an invariant across architectures, within asm.js code:
    $sp % StackAlignment = (AsmJSSizeOfRetAddr + masm.framePushed) % StackAlignment
  To achieve this on ARM, the first instruction of the asm.js prologue pushes lr without
  incrementing masm.framePushed.

(also on MIPS).  This comment would actually be nice to put above the x86/x64 AsmJSSizeOfRetAddr definitions too, changing the last sentence to:
  On (x86|x64) this naturally falls out of the fact that the 'call' instruction pushes
  the return address on the stack and masm.framePushed = 0 at the first instruction of
  the prologue.
Attachment #8439374 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/297840857bb7
https://hg.mozilla.org/mozilla-central/rev/d77c59d7df06
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.