846658 - BaselineCompiler: Optimize BinaryArith(Boolean vs. Int32) operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Mandreel was hitting this heavily.  Easy enough to knock off.  We just need to handle the simple cases for airthmetic Bool/Bool, Bool/Int, or Int/Bool operations: Add, Sub, BitOr, BitXor, BitAnd.

BitAnd is the only one getting hit by Mandreel, but the other cases are easy enough to handle in the same go.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Patch

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 719841 [details] [diff] [review]
Patch

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

This looks fine, but is it possible to just reuse the cache for int32 x int32 arithmetic, by making it an {int32,bool} x {int32,bool} cache in the way you've done here for ICBinaryArith_BooleanWithInt32?  It seems like that would avoid most of the new code.

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> Comment on attachment 719841 [details] [diff] [review]
> Patch
> 
> Review of attachment 719841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, but is it possible to just reuse the cache for int32 x
> int32 arithmetic, by making it an {int32,bool} x {int32,bool} cache in the
> way you've done here for ICBinaryArith_BooleanWithInt32?  It seems like that
> would avoid most of the new code.

This suggestion is good, and I went about trying to implement it..

I had forgotten that we decided to kind of micro-optimize our stub implementations, though.  So there are 3 different variants of the Int32xInt32 stubs, one for each architecture.  And they are all use architecture specific tricks (like using payload register directly and not retagging the ValueOperand for int32 adds on 32-bit archs).

I thought I was being all clever at first.. in retrospect, this seems to have been a premature optimization.  There's a lot of code complexity and I don't think we're gaining much from it.

I'm in favour of merging these two stub kinds into one codebase, but I'd do it the other way around: eliminating the highly micro-optimized per-architecture Int32 stubs and replacing them with more general, all-architecture stubs that handle (Int32|Boolean * Int32|Boolean * Int32Result|MaybeDoubleResult)

The same can be done for compares, which also currently use micro-optimized per-architecture stubs.

I'd like to leave both of those changes for a single clean-up patch afterward.

Comment 4

[Brian Hackett [Laid off!]]

Sounds good.

Comment 5

[Kannan Vijayan [:djvj]]

Pushed: https://hg.mozilla.org/projects/ionmonkey/rev/55d6c0e8f7cc
Waiting for tbpl before closing.