IonMonkey: Implement JSOP_SUB

RESOLVED FIXED

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I need this to run a benchmark with subtraction in it.
Comment on attachment 559299 [details] [diff] [review]
patch

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

::: js/src/ion/LIR-Common.h
@@ +475,2 @@
>  // Adds two integers, returning an integer value.
> +class LAddI : public LBinaryMath<0>

This might read more cleanly as "LBinaryMath<Temps(0)>" -- the current state requires readers to chase definitions.

::: js/src/ion/MIR.cpp
@@ +109,5 @@
> +      case MDefinition::Op_Add:
> +        ret.setNumber(lhs.toNumber() + rhs.toNumber());
> +        break;
> +      case MDefinition::Op_Sub:
> +        ret.setNumber(lhs.toNumber() - rhs.toNumber());

I don't see any place where overflow is handled -- likewise for Add and Mul.
Attachment #559299 - Flags: review?(sstangl) → review+
> ::: js/src/ion/MIR.cpp
> @@ +109,5 @@
> > +      case MDefinition::Op_Add:
> > +        ret.setNumber(lhs.toNumber() + rhs.toNumber());
> > +        break;
> > +      case MDefinition::Op_Sub:
> > +        ret.setNumber(lhs.toNumber() - rhs.toNumber());
> 
> I don't see any place where overflow is handled -- likewise for Add and Mul.

Okay, I see how this actually works now - the result is a double so it won't overflow, setNumber may re-cast to int, but we don't allow the MIR type to change (by GVN, it's too late).
http://hg.mozilla.org/projects/ionmonkey/rev/4129e39cffcd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.