575192 - JM: Fast-paths for JSOP_NEG.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: Implement double+int fast paths for negation.

The attached patch implements double (inline) and integer (ool) fast-paths for JSOP_NEG. Besides being a very minimal perf win, this patch is useful for FrameState redesign, since it exposes FrameState interface semantics in a friendlier way than the general arithmetic patch.

This patch has already been committed, but is being submitted for review.
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/9f6369809f66

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 454452 [details] [diff] [review]
Implement double+int fast paths for negation.

> 
>+
>     /*
>      * Throw away the entire frame state, without syncing anything.
>      */

Extra whitespace.

>+    /* FIXME: Should be private: hack during FrameState redesign. */
>+    inline void forgetReg(RegisterID reg);

Why is this necessary?

>+static const uint64 DoubleNegMask = 0x8000000000000000LLU;
>+void
>+mjit::Compiler::jsop_neg()

Empty line in between the two declarations is a bit more readable.

>+    /* Load type information into register. */
>+    MaybeRegisterID feTypeReg;
>+    //TODO: Optimize code emission if types are known.
>+    if (!frame.shouldAvoidTypeRemat(fe)) {

TODO better placed inside the first comment.

>+        /* Safe because only one type is loaded. */
>+        feTypeReg.setReg(frame.tempRegForType(fe));
>+        /* Don't get clobbered by copyDataIntoReg(). */
>+        frame.pinReg(feTypeReg.getReg());
>+    }

Second comment needs a preceding newline (this is true of all comments, except for those starting a block).
  

>+     * forgetReg() is thus exported for use here as a quick hack.
>+     *
>+     * Since this function is being integrated to serve as
>+     * example code during FrameState redesign, this comment is left
>+     * to point out more path merging issues.
>+     */
>+    frame.forgetReg(reg);

Why cant you use frame.freeReg() here? The protocol for "owned" registers (given by copy*IntoReg or ownRegFor*) is that they are either freed or pushed.

Nice patch - was around a 1% win on my box - didn't see anything slow down, and 3d-morph got an 8ms boost.

Comment 2

[Sean Stangl [:sstangl]]

Ah, I missed freeReg(). That's exactly what I was looking for, but evidently missed several times over.

Thanks for the quick review -- fixes pushed:
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/50075f6efa84