885183 - Use Push/Pop instead of explicitly manipulating the stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

Several places in the code explicitly reserve stack space and use it in a manner equivalent to a Push or Pop. Using Push and Pop allows targets like x86/x64 to optimize them to more efficient code.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

This patch also makes Pop more consistent with Push and adds Push and Pop methods for ARM to match their x86 counterparts.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 765179 [details] [diff] [review]
a proposed fix

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

::: js/src/ion/x64/Assembler-x64.h
@@ +357,5 @@
>          return label;
>      }
>  
> +    void pop(const FloatRegister &src) {
> +        addq(Imm32(sizeof(void*)), StackPointer);

This should be sizeof(double). The matching call to push() also needs sizeof(double) instead of (sizeof(void*)).

@@ +358,5 @@
>      }
>  
> +    void pop(const FloatRegister &src) {
> +        addq(Imm32(sizeof(void*)), StackPointer);
> +        movsd(Operand(StackPointer, 0), src);

This doesn't appear to match the semantics of a pop(). Shouldn't the load be from the top of the stack, and only then the stack pointer modified?

The equivalent push(const FloatRegister &src) code does appear to be written as expected.

::: js/src/ion/x86/Assembler-x86.h
@@ +284,5 @@
>      }
>  
> +    void pop(const FloatRegister &src) {
> +        addl(Imm32(sizeof(double)), StackPointer);
> +        movsd(Operand(StackPointer, 0), src);

Same issue here.

Comment 3

[Dan Gohman [:sunfish]]

(In reply to Sean Stangl [:sstangl] from comment #2)
> ::: js/src/ion/x64/Assembler-x64.h
> @@ +357,5 @@
> >          return label;
> >      }
> >  
> > +    void pop(const FloatRegister &src) {
> > +        addq(Imm32(sizeof(void*)), StackPointer);
> 
> This should be sizeof(double). The matching call to push() also needs
> sizeof(double) instead of (sizeof(void*)).

Makes sense.

> @@ +358,5 @@
> >      }
> >  
> > +    void pop(const FloatRegister &src) {
> > +        addq(Imm32(sizeof(void*)), StackPointer);
> > +        movsd(Operand(StackPointer, 0), src);
> 
> This doesn't appear to match the semantics of a pop(). Shouldn't the load be
> from the top of the stack, and only then the stack pointer modified?

Yes, good catch! I'll look into why this isn't causing trouble.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: an updated patch, following review feedback

(In reply to Dan Gohman from comment #3)
> (In reply to Sean Stangl [:sstangl] from comment #2)
> > ::: js/src/ion/x64/Assembler-x64.h

> > @@ +358,5 @@
> > >      }
> > >  
> > > +    void pop(const FloatRegister &src) {
> > > +        addq(Imm32(sizeof(void*)), StackPointer);
> > > +        movsd(Operand(StackPointer, 0), src);
> > 
> > This doesn't appear to match the semantics of a pop(). Shouldn't the load be
> > from the top of the stack, and only then the stack pointer modified?
> 
> Yes, good catch! I'll look into why this isn't causing trouble.

It turns out this isn't ever called. I needed it because I needed the generic Pop method for other purposes, and doing that while being consistent with the generic Push entailed having this.

Attached is a new patch which fixes this bug (as well as the sizeof(void *) thing). I left the pop method in, even though it's unused, as it's nice to keep Pop consistent with Push, but I'm happy to implement any fix you like.

Comment 5

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40f2d459d918

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40f2d459d918

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Dan Gohman from comment #0)
> Several places in the code explicitly reserve stack space and use it in a
> manner equivalent to a Push or Pop. Using Push and Pop allows targets like
> x86/x64 to optimize them to more efficient code.

Is this only for single pushes/pops? Or does this trick also applies for pushing more values. E.g. for saving the registers before calling?

Comment 8

[Dan Gohman [:sunfish]]

Yes, push/pop are good for multiple values. Modern processors have stack engines to optimize pushes and pops. Also, they are single-byte instructions, so they're good for code size.

The main downsides are that an explicit stack pointer increment or decrement after a sequence of pushes/pops costs an extra micro-op, and pushing with a memory operand from the stack also incurs an extra micro-op (source: Agner's micro-architecture manual, "Stack engine" sections). Also there is no push/pop for xmm registers. 

See also bugs 871811 and 868027.