Closed Bug 719135 Opened 12 years ago Closed 12 years ago

IonMonkey: Compile unspecialized SUB, MUL, DIV and MOD

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

We should probably rename MAddGeneric to MBinaryArithGeneric or something and call different stubs depending on the op.

We need this for Kraken gaussian-blur and some SS tests. Bug 715772 would help too, but this shouldn't be too hard and at some point we'll have to do it anyway.
Adding stubs to jsinterp.cpp and calling them in the interpreter loop caused a 40 ms SS regression, so I decided to add these operations to jsinterpinlines instead. This matches the other *Operation functions in jsinterpinlines.
Attachment #589846 - Flags: review?(dvander)
Attachment #589846 - Flags: review?(dvander) → review+
Comment on attachment 589846 [details] [diff] [review]
Patch for inbound

>--- a/js/src/jsinterpinlines.h
>+++ b/js/src/jsinterpinlines.h
>+AddOperation(JSContext *cx, const Value &lhs, const Value &rhs, Value *res)
>+{
>+        bool lIsString, rIsString;
>+        if ((lIsString = lval.isString()) | (rIsString = rval.isString())) {

This is pre-existing code, but isn't it trying to be too smart? It looks equivalent to

bool lIsString = lval.isString(), rIsString = rval.isString();
if (lIsString || rIsString) {
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d66257a176

This is just the first part so please don't close this bug.
(In reply to Ms2ger from comment #2)
>  
> This is pre-existing code, but isn't it trying to be too smart? It looks
> equivalent to
> 
> bool lIsString = lval.isString(), rIsString = rval.isString();
> if (lIsString || rIsString) {

Agreed. It may be interesting to compare the generated asm, and even if it's slightly better I don't think this kind of optimization helps much now that we have JITs.

r=me if you want to change it.
https://hg.mozilla.org/mozilla-central/rev/65d66257a176
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Attached patch PatchSplinter Review
Attachment #590689 - Flags: review?(nicolas.b.pierron)
Comment on attachment 590689 [details] [diff] [review]
Patch

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

Good job.

I like the idea of having the MAdd/MSub/... and LBinaryV, except that we may want LBinaryV supporting more functions.
During the review I verified that JSOP_* from JSOP_ADD to JSOP_MOD are packed, to make sure the switch case would not be replaced by series of if.  May be LBinaryV name should reflect this limitation.

::: js/src/ion/MIR.h
@@ +1094,5 @@
>          if (type() != ins->type())
>              return false;
>  
> +        if (isEffectful() || ins->isEffectful())
> +            return false;

Would it be better to limit congruentTo to identical aliasSet scopes instead of avoiding it, such as sharing an effectful call extracted from then-part and else-part of an if statement ?
Attachment #590689 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:pierron] from comment #7)
> 

Thanks for the quick review.

> 
> During the review I verified that JSOP_* from JSOP_ADD to JSOP_MOD are
> packed, to make sure the switch case would not be replaced by series of if. 
> May be LBinaryV name should reflect this limitation.

I just tested this with Clang 3.0 by adding some other ops to the switch. If there is a small number of "holes", it will still generate a jump table (the missing case values just jump to the default case). If there is a large number of holes, it generates a jump table and either adds jumps for the ops not handled by the table or it generates multiple jump tables.

> 
> Would it be better to limit congruentTo to identical aliasSet scopes instead
> of avoiding it, such as sharing an effectful call extracted from then-part
> and else-part of an if statement ?

We'd have to move the calls before the "if" but GVN won't do that (and GVN/LICM never move effectful instructions).
http://hg.mozilla.org/projects/ionmonkey/rev/ff07f7795262
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.