Closed Bug 817515 Opened 10 years ago Closed 10 years ago

BaselineCompiler: Compile NEG and BITNOT

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Need these for SS, eg nsieve-bits.
Patch for m-c to move JSOP_NEG code into NegOperation.
Attachment #687689 - Flags: review?(kvijayan)
Attached patch Part 2: Add ICSplinter Review
Attachment #687706 - Flags: review?(kvijayan)
Attachment #687689 - Flags: review?(kvijayan) → review+
Attachment #687706 - Flags: review?(kvijayan) → review+
Comment on attachment 687689 [details] [diff] [review]
Part 1: Add NegOperation

>-    /*
>-     * When the operand is int jsval, INT32_FITS_IN_JSVAL(i) implies
>-     * INT32_FITS_IN_JSVAL(-i) unless i is 0 or INT32_MIN when the
>-     * results, -0.0 or INT32_MAX + 1, are double values.
>-     */
>-    Value ref = regs.sp[-1];
>-    int32_t i;
>-    if (ref.isInt32() && (i = ref.toInt32()) != 0 && i != INT32_MIN) {
>-        i = -i;
>-        regs.sp[-1].setInt32(i);
>-    } else {
>-        double d;
>-        if (!ToNumber(cx, regs.sp[-1], &d))
>-            goto error;
>-        d = -d;
>-        if (!regs.sp[-1].setNumber(d) && !ref.isDouble())
>-            TypeScript::MonitorOverflow(cx, script, regs.pc);
>-    }
>+    RootedValue &val = rootValue0;

Why this ref to and use of rootValue0? There should be no need for a copy of the value to negate.

Check code-gen, did this really equate to the previous code (which should uave used a Value& ref but never mind)? I may well be picking on the previous version too!

/be
https://hg.mozilla.org/mozilla-central/rev/f8312a05c1a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(In reply to Brendan Eich [:brendan] from comment #5)
> 
> Why this ref to and use of rootValue0? There should be no need for a copy of
> the value to negate.
> 
> Check code-gen, did this really equate to the previous code (which should
> uave used a Value& ref but never mind)? I may well be picking on the
> previous version too!
> 
> /be

It couldn't use a reference, it has to keep the original value around after setting the result, to check if it's an int32 overflow:

if (!regs.sp[-1].setNumber(d) && !ref.isDouble())

ToNumber can trigger a GC, so this value has to be exactly rooted. Looking at other ops (e.g. SUB/MUL/DIV), the convention is to use rootValue0.
You need to log in before you can comment on or make changes to this bug.