589527 - JSOP_DOUBLE causes misaligned loads on trace

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Using njn's most excellent valgrind alignment-checking patch (bug 476122), it seems that JSOP_DOUBLE produces misaligned loads.  For example, given this loop:

var d = 1.1;
var x;
for (var i = 0; i < 10000; ++i) {
    d += 1.4;
}

running `TMFLAGS=recorder,native valgrind -v js -j test.js` the summary at the end produces

==11823== 9998 errors in context 111 of 111:
==11823== Misaligned 64-bit memory read
==11823==    at 0x4C63FA4: ???
==11823==    by 0x81B3E0A: js::ExecuteTrace(JSContext*, nanojit::Fragment*, js::TracerState&) (jstracer.cpp:6662)
==11823==  Address 0x4464d4c is 844 bytes inside a block of size 2,004 alloc'd
==11823==    at 0x402433F: calloc (vg_replace_malloc.c:467)
==11823==    by 0x81A16DF: js_calloc (jsutil.h:197)
==11823==    by 0x81A2D65: nanojit::Allocator::allocChunk(unsigned int) (jstracer.cpp:113)

and searching the log for 0x4464d4c produces the assembly:

  0x4c63f9c   movq xmm0,1472(edi)
  0x4c63fa4   movsd xmm1,(0x4464d4c) // =1.400000
  0x4c63fac   addsd xmm0,xmm1
  0x4c63fb0   movq 1472(edi),xmm0

Several SS tests show mis-aligned 64-bit reads from LIR buffers, so this might be a measurable speedup (e.g., math-partial-sums reports 100,000 such reads).

Comment 1

[Luke Wagner [:luke]]

Attachment: patch

I realized there is a simple fix.  Since we are loading doubles anyway, just load from the script.  On a Core 2, this is a 60% improvement on a micro-bench that adds and subtracts with double literals (but nothing on SS/V8).  On a Core i7, there is no change.

Comment 2

[Luke Wagner [:luke]]

Attachment: patch

Oops, dispWithin(JSScript) is wrong.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 485645 [details] [diff] [review]
patch

> TraceRecorder::record_JSOP_DOUBLE()
> {
>-    double d = consts[GET_INDEX(cx->regs->pc)].toDouble();
>-    stack(0, lir->insImmD(d));
>+    /* pd is a stable pointer into the JSScript. */
>+    double *pd = &consts[GET_INDEX(cx->regs->pc)].toDoubleRef();
>+    stack(0, lir->insLoad(LIR_ldd, INS_CONSTPTR(pd), 0, ACCSET_SCRIPT));

Nit: canonical name for double * is dp not pd (don't be talkin' no Hungarian 'round here).

Nice win!

/be

Comment 4

[Tom S. (please needinfo tschuster)]

Tracer has been removed.