Closed Bug 882008 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jruderman, Assigned: luke)

References

Details

(Keywords: testcase)

Attachments

(1 file)

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
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
> 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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(In reply to Jesse Ruderman from comment #2)
> Remind me where I can find this email thread?

asmjs@mozilla.com

> > 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).
Ok. If you'd rather change the spec, that's cool too, but you'll have to file ;)
THe github bug to change the spec is https://github.com/dherman/asm.js/issues/72
Attached patch fix and testSplinter Review
This patch applies on top of the stack in bug 880538 and removes Use completely.
Assignee: general → luke
Status: REOPENED → ASSIGNED
Attachment #769969 - Flags: review?(bbouvier)
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).
Attachment #769969 - Flags: review?(bbouvier) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/95523c5901df
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.