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.
Created attachment 42461 [details] The patch No performance impact on x86 / x86-64.
(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.
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.
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 on attachment 42461 [details] The patch r=me
(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.
Transmitting file data ...... Committed revision 50531.