Closed Bug 810603 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: djvj, Assigned: djvj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #680371 - Flags: review?(jdemooij)
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.
Attachment #680371 - Flags: review?(jdemooij)
Assignee: general → kvijayan
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.
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.
This adds a rudimentary type annotation to baseline StackValues.
Attachment #680371 - Attachment is obsolete: true
Attachment #716219 - Flags: review?(bhackett1024)
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.
Attachment #716219 - Flags: review?(bhackett1024) → review+
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.
Good to know - I'm actually glad to hear that, since patching inline paths does add a fair bit of complexity.
Skip ToBool ICs when input is a known boolean: https://hg.mozilla.org/projects/ionmonkey/rev/9b44dfd385df
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.