Use Push/Pop instead of explicitly manipulating the stack

RESOLVED FIXED in mozilla25

Status

()

--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Unassigned)

Tracking

Trunk
mozilla25
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 765179 [details] [diff] [review]
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.
Attachment #765179 - Flags: review?(sstangl)
(Reporter)

Updated

5 years ago
Blocks: 885186
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.
Attachment #765179 - Flags: review?(sstangl)
(Reporter)

Comment 3

5 years ago
(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.
(Reporter)

Comment 4

5 years ago
Created attachment 766695 [details] [diff] [review]
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.
Attachment #765179 - Attachment is obsolete: true
Attachment #766695 - Flags: review?(sstangl)

Updated

5 years ago
Attachment #766695 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/40f2d459d918
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(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?
(Reporter)

Comment 8

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.