521447 - Int/double confusion when computing sum with recursion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

print("hello");
function sum_slicing(array) {
  return array.length==0?0:array[0]+sum_slicing(array.slice(1));
}
for (var j = 0; j < 40; ++j) {
  if (j % 2 == 1) {
    print(sum_slicing([1,2,4,.1,.2]));
  }
}


Should say "7.3" consistently, but on some iterations it says "-6.277435978499887e+66".

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

The bug is that RecursiveSlotMap::adjustTypes() doesn't fix-up the specialization of the return value in case of int/double confusion.

This is not a trivial fix, I have to rework the SlotMap design a little and verify that both consumers of RecursiveSlotMap will be compatible.

I have a patch almost ready but it asserts with the test case in bug 521476, so it needs a little more work.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

When closing a loop, |SlotMap::adjustTypes()| invokes the tracker one last time to deal with promotion fixups. This is crucial because if slot X is INT32, and is compatible with a peer tree where slot X is DOUBLE, adjustTypes() will correctly link them together.

What this means: An exit typemap can have INT32 in a slot, but actually contain a double, if it is directly connected to a compatible peer.

Unfortunately the tracker doesn't work for propagating the return value with recursion, because the |sp| offset has a whacky computation. I tried to avoid that by manually and aggressively demoting the return value, but I didn't account for this in adjustTypes(), and trees got stitched together with incorrect types.

This patch puts return value propagation in the intended order, emulating what the tracker would do. There are two cases: 1) normal up recursion, where the return value must be written to the prior frame, and the current frame popped, and 2) "slurp" recursion, where the return value can be placed in the current frame.

While going over this code it seems like the entire TypeCheck_Promote path, recursive or not, is either unnecessary or poorly understood. Follow-up bug coming to address this.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: more context

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 405954 [details] [diff] [review]
more context

I think this patch can be simplified if I just address the TypeCheck_Promote oddities now - will post another shortly.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: better fix

Comment 7

[Andreas Gal :gal]

Comment on attachment 406105 [details] [diff] [review]
better fix


We should create some common code for this (in a follow-up patch):

>+        switch (returnType)
>+        {
>+          case TT_PSEUDOBOOLEAN:
>+          case TT_INT32:
>+            rval_ins = lir->insLoad(LIR_ld, lirbuf->sp, exit->sp_adj - sizeof(double));
>+            break;
>+          case TT_DOUBLE:
>+            rval_ins = lir->insLoad(LIR_ldq, lirbuf->sp, exit->sp_adj - sizeof(double));
>+            break;
>+          case TT_FUNCTION:
>+          case TT_OBJECT:
>+          case TT_STRING:
>+          case TT_NULL:
>+            rval_ins = lir->insLoad(LIR_ldp, lirbuf->sp, exit->sp_adj - sizeof(double));
>+            break;
>+          default:
>+            JS_NOT_REACHED("unknown type");
>+        }
>     }

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/31a4c08387f1

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/31a4c08387f1

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f9a98365c212