Last Comment Bug 684404 - Fix ARM assembler VFP instructions
: Fix ARM assembler VFP instructions
Status: RESOLVED FIXED
fixed-in-jaegermonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM All
: -- normal (vote)
: ---
Assigned To: Marty Rosenberg [:mjrosenb]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 669715
  Show dependency treegraph
 
Reported: 2011-09-02 16:36 PDT by Brian Hackett (:bhackett)
Modified: 2011-09-22 14:14 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add in necessary wrappers to encod vfp instructions correctly. (8.14 KB, patch)
2011-09-06 13:40 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
now with more fp-conversions (8.14 KB, patch)
2011-09-07 10:01 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
make the macro assembler use the correct registers for single values (3.38 KB, patch)
2011-09-08 13:44 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
fix massive vfp breakage (11.68 KB, patch)
2011-09-08 16:22 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-09-02 16:36:39 PDT
emitInst in the ARM assembler doesn't work correctly on floating point instructions, and can emit malformed instructions.
Comment 1 Marty Rosenberg [:mjrosenb] 2011-09-06 13:40:26 PDT
Created attachment 558588 [details] [diff] [review]
Add in necessary wrappers to encod vfp instructions correctly.

Should fix the issue.  However, there are some functions (namely the last three in the file) that are only referred to by their pre-UAL names, and I don't know what the UAL names are, so I cannot actually verify that the encoding is correct with the spec. I've guessed, and testing seems to not show anything horribly awry.
Comment 2 Brian Hackett (:bhackett) 2011-09-06 19:46:43 PDT
This patch seems to be broken for at least some combinations of registers when converting between doubles and int32s.  With this plus the patch in bug 669715, I looked at the first two failures I get with jit-tests --jitflags=mna.  I put simplified versions of these in my home directory on the ARM box, as failure-01.js and failure-02.js.  If you can figure out what's going on here that would be great.

failure-01.js

var x = 0;
var str = "a";
for (var i = 0; i < 10; ++i) {
    var y = str.charCodeAt(Infinity);
    print(y);
    x = y | 0;
    print(x);
}
assertEq(x, 0);

The second time around the loop (first time we are in the interpreter), we try to convert 'y' to an int32.  It is a NaN, but the code emitted by masm.branchTruncateDoubleToInt32 when called by Compiler::ensureInteger code passes the value through like it is an int32 and the 'y | 0' produces the wrong result.

failure-02.js

for (let i = 0; i != 30; i+=2) {
    print(i%4/2);
}

i%4 is a stub call which pushes an int32.  The DIV converts this to a double but always comes up with the value '0', so the result of the DIV is always zero rather than zero alternating with two.  Some disassembly:

=> 0x401809dc:	ldr	r8, [r10, #80]	; 0x50
=> 0x401809e0:	vmov	s0, r8
(gdb) p $r8
$10 = 2
=> 0x401809e4:	vcvt.f64.s32	d2, s2
(gdb) p $s0
$11 = 2.80259693e-45
(gdb) p $r8
$12 = 2
(gdb) p $s2
$13 = 0

It loads r8 from the stack and gets the right value, but d2 ends up as zero.
Comment 3 Marty Rosenberg [:mjrosenb] 2011-09-07 10:01:23 PDT
Created attachment 558856 [details] [diff] [review]
now with more fp-conversions

we were mis-handling the SN encoding scheme (0xf00 does not mask out bits 16-19, it masks out bits 8-13), and I believe the arguments for the two convert-dbl-to-int functions were mismatched.
Comment 4 Brian Hackett (:bhackett) 2011-09-07 20:45:40 PDT
Next VFP bug...  I don't know if this is a separate issue from this patch or not, but when branchTruncateToInt32 is used to compile Math.floor in the below testcase, it clobbers the source FP register.  The source is still live, and is written out to stack and ends up producing the wrong result for one of the floor() calls (the 'res' string will have a NaN in it).


function dofloor(v)
{
  var res = "";
  for (var i = 0; i < 10; i++) {
    var q = Math.floor(v + i);
    res += q + ",";
  }
  assertEq(res, "2147483642,2147483643,2147483644,2147483645,2147483646,2147483647,2147483648,2147483649,2147483650,2147483651,");
}
dofloor(0x7ffffffd + .5);


Disassembly for part of the branchTruncateToInt32 call (I added an extra mov(d1, d3) in a futile attempt to avoid clobbering d1, but the result is the same with or without this mov).


=> 0x40166118:	vmov.f64	d3, d1
(gdb) p $d1   
$10 = 2147483647.5
(gdb) p $d3
$11 = 2147483646.5
(gdb) stepi
=> 0x4016611c:	vcvt.s32.f64	s3, d3
(gdb) p $d1
$12 = 2147483647.5
(gdb) p $d3
$13 = 2147483647.5
(gdb) p $s3
$14 = 27.9999981
(gdb) stepi
3: x/i $pc
=> 0x40166120:	vmov	r7, s3
(gdb) p $s3
$15 = nan(0x7fffff)
(gdb) p $d3
$16 = 2147483647.5
(gdb) p $d1
$17 = nan(0xfffffffe00000)
Comment 5 Marty Rosenberg [:mjrosenb] 2011-09-08 13:44:13 PDT
Created attachment 559270 [details] [diff] [review]
make the macro assembler use the correct registers for single values

the latest issue is that on arm, the single registers alias the double registers, but not in a 1-1 mapping; d1 aliases both s2 and s3.  In order to convert an integer to a double, we need to first copy the integer register into a single register, then convert the value in the single register into a double register.  We attempt to re-use the dest register as the single register that we copy the value into.  Unfortunately, we just re-use the number. In this case, we try to use d3 for the single value.  d3 is represented as "3", so we were in fact using s3, (which stomped on the value d1), rather than d6 or d7, which would have stomped on d3.
Comment 6 Marty Rosenberg [:mjrosenb] 2011-09-08 16:22:16 PDT
Created attachment 559318 [details] [diff] [review]
fix massive vfp breakage

both previous patches rolled into one, with mq-goodness
Comment 7 Brian Hackett (:bhackett) 2011-09-08 18:16:21 PDT
http://hg.mozilla.org/projects/jaegermonkey/rev/a887241aed3a
Comment 8 Brian Hackett (:bhackett) 2011-09-22 14:14:44 PDT
https://hg.mozilla.org/mozilla-central/rev/c02868b913d5

Note You need to log in before you can comment on or make changes to this bug.