810603 - BaselineCompiler: Add inline paths for JSOP_ADD, JSOP_LT, and JSOP_IFNE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

JSOP_ADD and JSOP_LT need inline paths that check for integer cases.  Maybe we can check for doubles later.

JSOP_IFNE needs inline paths handling the common case where the input is a bool.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Inline paths for ADD, LT, and IFNE.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 680371 [details] [diff] [review]
Inline paths for ADD, LT, and IFNE.

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

Canceling review request because inlining is x86-specific.  Will fix and resubmit.

Comment 3

[Kannan Vijayan [:djvj]]

Did some measurements based on adding an Int32 inline path for JSOP_ADD today.

On 32-bit x86 the difference with just the JSOP_ADD inlined was negligible and well within the noise threshold.

On x64, the difference was more noticeable - the BL+Ion went from being 6.4% faster than trunk, to 7.1% faster.

I think if we inline all of the major Int32 ops in one go (Arithmetic, Compare, and Bitops), then we will see a noticeable gain.  However, given the modest improvements I'm seeing with just JSOP_ADD, I think it's better to revisit this at a later point when we've exhausted the low hanging fruit, of which there seem other more likely candidates.

Comment 4

[Kannan Vijayan [:djvj]]

Realized earlier today that we're not actually faster than trunk on BL+Ion.  I was measuring '--no-jm' vs. '--no-baseline' on the same binary, which measures with the lowered useCount on both, and doesn't use the optimal useCount for trunk.

We're still about 5% back or so trunk.  The Int32 inline paths for BinaryArith, Compare and Bit ops should not be that much of a lift, so I'll just do them now and get it over with.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: Skip ToBool ICs when we know that the top of the stack is boolean.

This adds a rudimentary type annotation to baseline StackValues.

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 716219 [details] [diff] [review]
Skip ToBool ICs when we know that the top of the stack is boolean.

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

::: js/src/ion/BaselineFrameInfo.h
@@ +136,5 @@
>          kind_ = Register;
>          *data.reg.reg.addr() = val;
> +        knownType_ = JSVAL_TYPE_UNKNOWN;
> +    }
> +    void setRegister(const ValueOperand &val, JSValueType knownType) {

Can you just use a single setRegister(ValueOperand, JSValueType knownType = JSVAL_TYPE_UNKNOWN) function?

@@ +246,5 @@
>      inline void push(const ValueOperand &val) {
>          StackValue *sv = rawPush();
>          sv->setRegister(val);
>      }
> +    inline void push(const ValueOperand &val, JSValueType knownType) {

Ditto.

Comment 7

[Kannan Vijayan [:djvj]]

So here's what I think the story on inline paths for ops: they're not worth it.

After testing inline paths for all the Compare ops, along with Add, Sub, and shift operators.. I'm consistently getting slightly lower scores with inlining on than off.  After much scratching of my head and trying various things, I ran the benches with ion disabled, and lo! there was a modest improvement.

At a useCount of 1000 before we enter Ion, the extra time we spend checking for and compiling inline paths in certain cases simply doesn't provide enough of a delta to justify the extra compilation cost.

Comment 8

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

Good to know - I'm actually glad to hear that, since patching inline paths does add a fair bit of complexity.

Comment 9

[Kannan Vijayan [:djvj]]

Skip ToBool ICs when input is a known boolean: https://hg.mozilla.org/projects/ionmonkey/rev/9b44dfd385df