Some of the cleanups and tests we discussed during bug 1022142 debugging.
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.
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)
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 on attachment 8439229 [details] [diff] [review] 2. Moar asm.js => ion testing Nice!
Attachment #8439229 - Flags: review?(luke) → review+
(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.
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.
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+
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.