882008 - OdinMonkey: subtraction (binary -) should take doublish, not double

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

http://asmjs.org/spec/latest/ says subtraction takes doublish arguments, but OdinMonkey expects doubles.

~~~ testcase with foreign functions (unknown is a subtype of doublish)

var f = (function(stdlib, foreign) {
  "use asm";
  var identity = foreign.identity;
  function f()
  {
    return +(identity(5.0) - identity(3.0));
  }
  return f;
})(this, {identity:function(x){return x}});
print(f());

warning: asm.js type error: unknown is not a subtype of double

~~~ testcase with typed arrays (reading from a float*array returns doublish)

var heap = new ArrayBuffer(4096);
var doubles = new Float64Array(heap);
doubles[1] = 5.0;
doubles[0] = 3.0;
var g = (function(stdlib, foreign, heap) {
  "use asm";
  var doubles = new stdlib.Float64Array(heap);
  function g()
  {
    return +(doubles[1] - doubles[0]);
  }
  return g;
})(this, null, heap);
print(g());

warning: asm.js type error: doublish is not a subtype of double

Comment 1

[Luke Wagner [:luke]]

In this case, it is the spec that is missing an update that we emailed about earlier.

The problem is that we need to propagate "how do I get coerced?" from parent expressions to child expressions.  If - takes doublish and intish (because of the special +/- rule), that makes it ambiguous (or at least nuanced) what coercion is applied to an ffi call.  So, earlier, I proposed changing - to take double to make it simple.  Filing a github issue on the spec would be nice, though.

Comment 2

[Jesse Ruderman]

> In this case, it is the spec that is missing an update that we emailed about
> earlier.

Remind me where I can find this email thread?

> If - takes doublish and intish (because of the special +/- rule), that makes it 
> ambiguous (or at least nuanced) what coercion is applied to an ffi call.

The special +/- rule takes int (not intish) so I don't think there's ambiguity here.

Comment 3

[Luke Wagner [:luke]]

(In reply to Jesse Ruderman from comment #2)
> Remind me where I can find this email thread?

[e-mail]

> > If - takes doublish and intish (because of the special +/- rule), that makes it 
> > ambiguous (or at least nuanced) what coercion is applied to an ffi call.
> 
> The special +/- rule takes int (not intish) so I don't think there's
> ambiguity here.

Ah, good, I thought it took intish.  In that case, I think you're right (although this will require a bit of tweaking of how we propagate uses to differentiate Add from Sub).

Comment 4

[Jesse Ruderman]

Ok. If you'd rather change the spec, that's cool too, but you'll have to file ;)

Comment 5

[cscott]

THe github bug to change the spec is https://github.com/dherman/asm.js/issues/72

Comment 6

[Luke Wagner [:luke]]

Attachment: fix and test

This patch applies on top of the stack in bug 880538 and removes Use completely.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 769969 [details] [diff] [review]
fix and test

Review of attachment 769969 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! We can now get rid of the Use class!

::: js/src/ion/AsmJS.cpp
@@ +3612,5 @@
>  
>  static bool
>  CheckCall(FunctionCompiler &f, ParseNode *call, RetType retType, MDefinition **def, Type *type)
>  {
> +    JS_CHECK_RECURSION(f.cx(), return false);

That's a good idea to add this check, but shouldn't we add it at several other places? The C recursion limit could also be reached in CheckExpr, for instance.

@@ +3934,5 @@
> +            return false;
> +        rhsNumTerms = 0;
> +    }
> +
> +    unsigned numTerms = lhsNumTerms + rhsNumTerms + 1;

Shouldn't this be named numOperations / numOperators? If we evaluate "1+2", numTerms == 1 while we have two terms. But we have indeed one operation.

If we want to keep the number of terms instead, we can just set {lhs,rhs}NumTerms to 1 in the above else branches and just add them to obtain numTerms (no need for +1).

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> That's a good idea to add this check, but shouldn't we add it at several
> other places? The C recursion limit could also be reached in CheckExpr, for
> instance.

It's already present in CheckExpr/CheckStatement.  The reason for the additional two here is because they allow deep recursion that *doesn't* funnel through CheckExpr/CheckStatement.

> Shouldn't this be named numOperations / numOperators? If we evaluate "1+2",
> numTerms == 1 while we have two terms. But we have indeed one operation.

Good point!  Indeed, 'operators' is really what the spec mandates we limit here.

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95523c5901df

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95523c5901df