Bug 31104 - Refactor x86-specific behaviour out of the JIT.
Summary: Refactor x86-specific behaviour out of the JIT.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-04 00:26 PST by Gavin Barraclough
Modified: 2009-11-04 16:00 PST (History)
2 users (show)

See Also:


Attachments
The patch (15.09 KB, patch)
2009-11-04 00:33 PST, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2009-11-04 00:26:08 PST
The macro assembler is currently inconsistent in it's handing of unordered results in double comparisons.
In the case of equal and less-than comparisons unordered operands will result in the  branch being taken, but in the case of not-equal and greater-than  comparison unordered operands will result in the branch not being taken.
Factor the branch conditions out into two sets – explicitly calling out those that will branch if the operands are unordered.

Functionality on ARM/ARMv7 backends should be unchanged by this patch – I've assumed that their double comparisons were operating in the fashion required by the JIT.

Also, refactor the double to integer conversion out of the the JIT and into the MacroAssembler (into branchConvertDoubleToInt32).

In the case of non-JSVALUE32_64 builds the 'optimization' to sometimes convert values back to integers was broken (it was missing the move of the converted value back to fpRegT1 before comparing).  'Fixing' the optimization resulted in no performance change, removing the broken code resulted in a fractional improvement, so it is gone.

In the case of JSVALUE32_64 the conversion does appear to be of benefit, but was oddly guarded.  The conversion back to an integer immediate would not take place if the left had operand to the modulus was the immediate integer 1 (or hypothetically zero/negative values).  The only cases this exclusion had an effect was onmath-partial-sums & math-spectral-norm, and performance testing, including focussing on of these benchmarks, shows no regression from removing this check.  Since it is no longer providing any benefit, removing to simplify the code.
Comment 1 Gavin Barraclough 2009-11-04 00:33:39 PST
Created attachment 42461 [details]
The patch

No performance impact on x86 / x86-64.
Comment 2 Zoltan Herczeg 2009-11-04 00:53:34 PST
(In reply to comment #1)
> Created an attachment (id=42461) [details]
> The patch
> 
> No performance impact on x86 / x86-64.

The patch looks good. I wish I could give an r+ :)

On ARM (both version), we also need to add those "DoubleConditionBit"-s and modify the double comparison functions accordingly. (Naturally this fix breaks the ARM ports) I suggest change this bugzilla entry to "multipatch", and add those ARM fixes here as well. We can do it now.
Comment 3 Gavin Barraclough 2009-11-04 01:26:18 PST
Quick note, as discussed in irc, this shouldn't break ARM.  We should land matching changes (adding the new double conditions & conversion methods), but these can be done in separate patches.
Comment 4 Zoltan Herczeg 2009-11-04 12:12:47 PST
I am just wondering whether we can change DoubleConditionBitSpecial to compile ASSERT, since it is only used by an ASSERT (altough I feel it will have no performance impact).

Good to know that this function is not used anymore:
Jump branchDouble(DoubleCondition cond, FPRegisterID left, Address right)
I sometimes feel there are some unused MA instructions. Is there a way to detect and remove them?
Comment 5 Oliver Hunt 2009-11-04 13:38:41 PST
Comment on attachment 42461 [details]
The patch

r=me
Comment 6 Gavin Barraclough 2009-11-04 15:56:08 PST
(In reply to comment #4)
> I am just wondering whether we can change DoubleConditionBitSpecial to compile
> ASSERT, since it is only used by an ASSERT (altough I feel it will have no
> performance impact).

DoubleConditionBitSpecial isn't only used by an assert, though it's other use is slightly obscured.  It is also used to distinguish DoubleEqual from DoubleEqualOrUnordered in the check "(cond == DoubleEqual)" (and the same is true for NotEqual, in the else if case).  The ones marked special need an extra check re. unordered operands, the 'unspecial' ones need no extra check & can be performed with a single compare.

> Good to know that this function is not used anymore:
> Jump branchDouble(DoubleCondition cond, FPRegisterID left, Address right)
> I sometimes feel there are some unused MA instructions. Is there a way to
> detect and remove them?

Hmmm, agreed, sounds like a good idea, but I'm not sure what a great automated way would be.  Really what you want here is not dynamic coverage, but static – maybe the compiler/linker could kick out a list of functions that aren't called.  Though I guess this would be a little tricky – since these are inline functions I doubt the linker knows anything about them, but since some methods may be used from one compilation unit and others from another the compiler won't know.  So I'm not sure the standard tool-chain will be able to help here, sounds like we'd need a specific static code coverage tool? - I'm not aware of any, but I would imagine there is something out there that will do the job.
Comment 7 Gavin Barraclough 2009-11-04 16:00:01 PST
Transmitting file data ......
Committed revision 50531.