Jan de Mooij [:jandem] Assignee
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.5 (KHTML, like Gecko) Chrome/6.0.486.0 Safari/534.5
Build Identifier: 

JM tip fails the following test case:
----------
function mul(x, y) { return x * y; }

var a = mul(-5, 0);
var b = Infinity / a;
print(b < 0 ? 'PASS' : 'FAIL');
----------
This may also apply to other binary operators.

Reproducible: Always

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 14 years ago

I thought we had trace-tests for this too...

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 14 years ago

(In reply to comment #1)
> I thought we had trace-tests for this too...

It works correctly when the mul is constant folded:
--------
function f() { 
  var a = -5;
  var b = 0;
  print(Infinity / (a * b));
}
--------
Maybe that's happening to the trace-tests...

	Jan de Mooij [:jandem] Assignee
	Comment 3 • 14 years ago

Taking this, will attach a patch tomorrow.

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 14 years ago

This patch adds a number of tests and passes all of them, except the last one. I haven't found the reason yet but I think the FrameState is messed up somehow.

This line:
  masm.loadPayload(frame.addressOf(regs.resultHasRhs ? rhs : lhs), regs.result);

is supposed to load the original operand from memory. Looking at the asm reveals that it loads another value from the stack, which is weird.

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 14 years ago

The problem is defninitely with this line: 
  masm.loadPayload(frame.addressOf(regs.resultHasRhs ? rhs : lhs), regs.result);

I think that in my testcase the operands are arguments and only stored in registers. Reloading them like this won't work.

Some help here would be appreciated.

	Jan de Mooij [:jandem] Assignee
	Comment 6 • 14 years ago

This fixes the constant-zero operand (x * 0) case, with tests.

The last test still fails, otherwise this is close.

	Jan de Mooij [:jandem] Assignee
	Comment 7 • 14 years ago

This moves more negative-zero code to stubcc, to make the inline path more concise and readable, may also help branch prediction a tiny tiny bit. 

For the constant-0 operand case we could do a xor and skip the mul like in patch v2. This is rare though and complicates the rest of the code (have to check if overflow jump is set, etc.), so this patch just does an imul to keep it simple.

Passes trace-tests and attached tests. Let's hope SS/V8 are not too much affected.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 8 • 14 years ago

Comment on attachment 466264 [details] [diff] [review]
Patch v3

Looks great, thanks for catching & taking this, Jan. Also thanks for the tests, kind of shocking that we weren't failing anything already.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 9 • 14 years ago

http://hg.mozilla.org/projects/jaegermonkey/rev/fe75f7100230