589526 - FASTCALL can misalign doubles

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Luke Wagner [:luke]]

Attachment: hack

I took njn's most excellent misalignment-reporting valgrind patch (bug 476122) out for a spin on 3d-cube and saw:

==11295== 157080 errors in context 92 of 93:
==11295== Misaligned 64-bit memory read
==11295==    at 0x805FEA3: js_Array_dense_setelem_double(JSContext*, JSObject*, int, double) (jsarray.cpp:909)
==11295==    by 0x4C62A84: ???
==11295==    by 0x4C628B6: ???
==11295==    by 0x8178703: js::ExecuteTree(JSContext*, js::TreeFragment*, unsigned int&, js::VMSideExit**, js::VMSideExit**) (jstracer.cpp:6662)
==11295==  Address 0xbed23b44 is on thread 1's stack

==11295== 157080 errors in context 93 of 93:
==11295== Misaligned 64-bit memory write
==11295==    at 0x4C62C4D: ???
==11295==    by 0x4C62A84: ???
==11295==    by 0x4C628B6: ???
==11295==    by 0x8178703: js::ExecuteTree(JSContext*, js::TreeFragment*, unsigned int&, js::VMSideExit**, js::VMSideExit**) (jstracer.cpp:6662)
==11295==  Address 0xbed23b44 is on thread 1's stack

The pairing of 157080 reads/writes to a stack location seemed to suggest that this was the double parameter to js_Array_dense_setelem_double.  Switching the order of the int32 and double parameter makes the valgrind diagnostics go away and makes this micro-benchmark 5% faster on my 1.5GHz Pentium M:

var arr = [1.1];
var bef = new Date();
for (var i = 0; i < 10000000; ++i)
    arr[0] = arr[0] + .1;

The change is in the noise for SS as a whole, but makes the 3d tests .8% (1-2ms) faster.

The patch is a joke, but attached for reference.  We should probably inspect the other double-taking natives as well.

Comment 1

[Werner Sharp]

Please see bug 409216 for some further investigation of unaligned double access in TR.  I'm assuming this bug is using MSVC which only supports 4 byte stack alignment and can easily unalign doubles.  From my investigation, the worse performance problem with unaligned doubles is when the load/store spans a cache line.  VTUNE will catch this case as L1D_SPLIT.STORES and L1D_SPLIT.LOADS.  The patch is 409216 forces 8-byte stack alignment before calling JITed code and also keeps the JIT stack 8-byte aligned.  Even with these changes, we can get code using unaligned doubles.  (see frame pointer fix patch for one example).

Comment 2

[Luke Wagner [:luke]]

This is actually from GCC.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.

Comment 4

[Edwin Smith]

I'd like to move this over to tamarin if nobody minds.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.