Last Comment Bug 709240 - IonMonkey: Compile JSOP_NOT
: IonMonkey: Compile JSOP_NOT
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: kosver
:
Mentors:
Depends on:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-12-09 12:02 PST by Jan de Mooij [:jandem] (PTO until July 31)
Modified: 2012-02-13 17:27 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (8.68 KB, patch)
2012-01-06 09:51 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v1 (9.66 KB, patch)
2012-01-06 13:23 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
JSOP_NOT by dolfje (12.79 KB, patch)
2012-01-07 12:18 PST, kosver
no flags Details | Diff | Splinter Review
v2 (13.24 KB, patch)
2012-01-10 07:35 PST, kosver
no flags Details | Diff | Splinter Review
v2 part2: implement fastpath for double not (7.59 KB, patch)
2012-01-10 14:47 PST, kosver
no flags Details | Diff | Splinter Review
v2 part2: implement fastpath for double not (7.60 KB, patch)
2012-01-11 03:24 PST, kosver
no flags Details | Diff | Splinter Review
v2 part3: remove not before compare and test (4.80 KB, patch)
2012-01-11 08:08 PST, kosver
no flags Details | Diff | Splinter Review
v2 part4: implement LCompareBAndBranch (4.83 KB, patch)
2012-01-11 08:13 PST, kosver
no flags Details | Diff | Splinter Review
v3 (10.65 KB, patch)
2012-01-14 09:16 PST, kosver
no flags Details | Diff | Splinter Review
v3 part2: implement fastpath for double not (7.71 KB, patch)
2012-01-14 09:17 PST, kosver
no flags Details | Diff | Splinter Review
v3 part3: remove not before compare and test (4.79 KB, patch)
2012-01-14 09:19 PST, kosver
no flags Details | Diff | Splinter Review
v3 part4: implement LCompareBAndBranch (4.90 KB, patch)
2012-01-14 09:19 PST, kosver
no flags Details | Diff | Splinter Review
v4 (13.23 KB, patch)
2012-01-31 05:15 PST, kosver
cdleary: review+
Details | Diff | Splinter Review
v4 part2: implement fastpath for double not (10.88 KB, patch)
2012-01-31 05:50 PST, kosver
no flags Details | Diff | Splinter Review
v4 part3: remove not before compare and test (5.15 KB, patch)
2012-01-31 05:50 PST, kosver
no flags Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] (PTO until July 31) 2011-12-09 12:02:34 PST
We still need to implement JSOP_NOT. 

The specialized version could be lowered like this:

MNot(x):
- boolean: x xor 1
- int32: LCompare(x, 0)
- string: LCompare(LStringLength(x), 0)
- object: 0
- undefined/null: 0
Comment 1 Jan de Mooij [:jandem] (PTO until July 31) 2011-12-09 12:05:46 PST
(In reply to Jan de Mooij (:jandem) from comment #0)
> - undefined/null: 0

Ehm 1 of course.
Comment 2 Tom Schuster [:evilpie] 2012-01-06 09:51:07 PST
Created attachment 586462 [details] [diff] [review]
WIP

not sure if i keep that design
Comment 3 Tom Schuster [:evilpie] 2012-01-06 13:23:24 PST
Created attachment 586548 [details] [diff] [review]
v1

What do you think of this version?
It uses MNot for everything, in adjustInputs it changes the input if it's a string to the string length. It does the constant folding for constants and object in foldsConstants and lowers To LCompare and LBitOp (albeit using a hack).

I also realized that we should optimize if (!a) cases.
I wasn't sure if I need to use the Type Oracle at all.

Overall, I think this version is cleaner and better abstracted. And with rewriting everything to the eg. MCompare the optimization in visitTest wouldn't be so clean, I think.
Comment 4 Tom Schuster [:evilpie] 2012-01-06 16:53:39 PST
Comment on attachment 586548 [details] [diff] [review]
v1

Looking at this, this is actually ready, sans handling of Value.
Comment 5 kosver 2012-01-07 12:18:20 PST
Created attachment 586722 [details] [diff] [review]
JSOP_NOT by dolfje

I implemented the JSOP_NOT opcode to introduce myself to the codebase of ionmonkey. 
It took me a little bit longer and now there is already a patch.
There are some differences between the patches, so I added mine patch to compare them.

Some of the key changes are:
- I used the TypePolicy for asking Integers when the Oracle returns MIRType_Any, MIRType_Value.
  I think the not operator will be used most for Integers or Booleans, therefor the preference.
- With MIRType_String, I only decompose the instruction into MStringLength and MNot in the Lowering phase
- I also doesn't change MNot to MConstanct for MIRType_Object in the foldsTo of MNot.
  I think the types aren't fixed yet. (Correct me if I'm wrong)
- Mine version implements also JSOP_NOT for MIRType_Null and MIRType_Undefined

What's missing:
- I don't have a fast path for JSOP_NOT before JSOP_TEST like the other patch
- I think it should also be good to implement a JSOP_NOT after JSOP_NOT optimalisation
Comment 6 Tom Schuster [:evilpie] 2012-01-09 06:25:54 PST
Clearing review, because kosver patch overall looks better. Next time, I would be awesome if you would comment in the bug, if you are working on it.
Comment 7 kosver 2012-01-10 07:35:19 PST
Created attachment 587306 [details] [diff] [review]
v2

Version applies to trunk and is ready.
I will make Fast paths for NOT NOT and NOT COMPARE, but in follow-up patches.
Comment 8 Paul Wright 2012-01-10 08:33:40 PST
Who should review this?
Comment 9 kosver 2012-01-10 14:47:56 PST
Created attachment 587485 [details] [diff] [review]
v2 part2: implement fastpath for double not

Added a fastpath for double not. Next on my list: not before compare/test
Comment 10 kosver 2012-01-11 03:24:33 PST
Created attachment 587647 [details] [diff] [review]
v2 part2: implement fastpath for double not

fix bug with three not's in row
Comment 11 kosver 2012-01-11 08:08:20 PST
Created attachment 587703 [details] [diff] [review]
v2 part3: remove not before compare and test
Comment 12 kosver 2012-01-11 08:13:41 PST
Created attachment 587704 [details] [diff] [review]
v2 part4: implement LCompareBAndBranch

Because MCompare will change Boolean inputs to Int32 inputs, it will add an MToInt32 before it inputs. Therefor MNot fastpath before Compare will not trigger.

Patch part4 adds support for MIRType_Boolean in MCompare. This will also give performance enhancement because the unneeded transformation to Int32 is removed.
Comment 13 David Anderson [:dvander] 2012-01-13 16:20:26 PST
Comment on attachment 587306 [details] [diff] [review]
v2

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

::: js/src/ion/IonBuilder.cpp
@@ +3130,5 @@
> +    
> +    // The NOT opcode is more usefull with boolean and integers,
> +    // so if oracle doesn't know, bias towards integers
> +    if (type == MIRType_Any || type == MIRType_Value) {
> +        // Create instructions

I don't think you actually need anything in the first part of this |if|. The ToInt32 insertion can (and should) be done in the TypePolicy. So you can remove the test and just have the code in the else branch :)

Then after you could say: if (type >= MIRType_Value) ... ins->infer(MIRType_Int32)

::: js/src/ion/Lowering.cpp
@@ +797,5 @@
> +      }
> +
> +      case MIRType_String: { 
> +        MStringLength *length = new MStringLength(op);
> +        ins->block()->insertBefore(ins, length);

I'm torn over this approach. On one hand, it's really, really too late to generate MIR. The point of MIR is high-level optimizations, and you're missing those at this stage. This also makes MIR:LIR not 1:1. On the other hand, trying to generate multiple LIR at once is really painful, thus necessitating these MIR hacks.

So, here's a few options:
 (1) Decompose these MIR in IonBuilder instead. I would definitely do this for the string case.
 (2) Generate unique LIR nodes, like LNotI, LNotString.
 (3) Remove the ins->block()->insertBefore calls. We consider the MIR nodes to be temporaries to assist in generating LIR.
 (4) Decompose LIR manually, adding helpers if needed. The annoying part is that use*() always operates on MIR, so you'd have to add a few for LIR.

I leave it to you which one to choose, I've ranked them in personal preference though.

::: js/src/ion/MIR.cpp
@@ +456,5 @@
> +        /* ValueToBoolean can cause no side-effects, so this is safe */
> +        return MConstant::New(BooleanValue(!js_ValueToBoolean(v)));
> +    }
> +    
> +    // NOT of an object is always 0

s/0/false

@@ +461,5 @@
> +    if (specialization_ == MIRType_Object) {
> +        return MConstant::New(BooleanValue(false));
> +    }
> +    
> +    // NOT of an undefined or null value is always 1

s/1/true
Nit: You can also remove the braces on these two |if|s, since they are single-line.

::: js/src/ion/TypePolicy.cpp
@@ +72,5 @@
> +TryUnboxPolicy::adjustInputs(MInstruction *ins)
> +{
> +    // Cannot Unbox  
> +    if (specialization_ >= MIRType_Value)
> +        return true;

So, with the simpler code in IonBuilder, you can remove this test here --

@@ +79,5 @@
> +        MDefinition *in = ins->getOperand(i);
> +        
> +        // Is not the proper input, so unbox
> +        if (in->type() != specialization_) {
> +            MInstruction *replace = MUnbox::New(in, specialization_, MUnbox::Fallible);

And this here has to change a little. Something like this:
  * Is the input a value? If so, unbox it as my specialization.
  * Otherwise, change my specialization (this is okay since it doesn't affect the rest of the method).

::: js/src/jit-test/tests/ion/bug709240.js
@@ +87,5 @@
> +  assertEq(k(null), true);
> +  assertEq(k(null), true);
> +}
> +// Check bailouts
> +checkAll (k);

Awesome! Thanks for writing all these tests!
Comment 14 kosver 2012-01-14 09:16:35 PST
Created attachment 588641 [details] [diff] [review]
v3
Comment 15 kosver 2012-01-14 09:17:46 PST
Created attachment 588642 [details] [diff] [review]
v3 part2: implement fastpath for double not
Comment 16 kosver 2012-01-14 09:19:07 PST
Created attachment 588643 [details] [diff] [review]
v3 part3: remove not before compare and test
Comment 17 kosver 2012-01-14 09:19:50 PST
Created attachment 588644 [details] [diff] [review]
v3 part4: implement LCompareBAndBranch
Comment 18 kosver 2012-01-14 09:23:07 PST
I've updated the patches to the recommendations of comment #13.

I've put part 1 as review, but all patches are ready. Should I put them all into review? Or wait till part 1 has positive review?
Comment 19 Jan de Mooij [:jandem] (PTO until July 31) 2012-01-27 01:14:15 PST
Review ping. Not my patch but this blocks several SS/Kraken tests.
Comment 20 David Anderson [:dvander] 2012-01-30 18:00:30 PST
Comment on attachment 588641 [details] [diff] [review]
v3

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

Nikos, this patch looks good but I'd like to make sure the untyped case gets handled generically. If you're willing, I can finish this patch up and check it in for you tomorrow (not sure how much free time you have, and it looks like this will help a bunch of benchmarks :)

::: js/src/ion/Lowering.cpp
@@ +777,5 @@
> +    
> +    // These opcodes are already changed into MConstant in MIR-foldsTo-phase
> +    JS_ASSERT(op->type() != MIRType_Undefined && 
> +              op->type() != MIRType_Null && 
> +              op->type() != MIRType_Object);

GVN is optional (--ion-gvn=off), so we need to do something here, like just duplicate the constant folding (lower to LInteger).

::: js/src/ion/MIR.cpp
@@ +473,5 @@
> +{
> +    // The NOT opcode is more usefull with boolean and integers,
> +    // so if oracle doesn't know, bias towards integers
> +    if (type >= MIRType_Value)
> +        specialization_ = MIRType_Int32;

If we don't know the type at all, we should leave it as despecialized and just call js_ValueToBoolean(). Otherwise we run the risk of compiling code that will always bailout.

@@ +477,5 @@
> +        specialization_ = MIRType_Int32;
> +    else
> +        specialization_ = type;
> +
> +    printf ("%s\n", StringFromMIRType(specialization_));

^-- left-over debug spew
Comment 21 kosver 2012-01-31 05:15:29 PST
Created attachment 593054 [details] [diff] [review]
v4

I have time this week, so updated my patch:
- Added LNotV, uses CallVM to call js_ValueToBoolean
- Added LNotI. Couldn't use LCompareI anymore, because it's removed in trunk.
- Make sure it works without GVN
- Removed debug spew
Comment 22 kosver 2012-01-31 05:50:10 PST
Created attachment 593060 [details] [diff] [review]
v4 part2: implement fastpath for double not
Comment 23 kosver 2012-01-31 05:50:44 PST
Created attachment 593061 [details] [diff] [review]
v4 part3: remove not before compare and test
Comment 24 Chris Leary [:cdleary] (not checking bugmail) 2012-02-03 04:09:53 PST
Bump! What can I do to help us get this opcode implementation in?
Comment 25 kosver 2012-02-03 09:41:56 PST
It should get reviewed. I don't foresee any remarks on the patches. Also part2 and part3 are ready, but are just additions (Some fastpaths). It can be inserted without those.

I will be gone starting from tomorrow, so if there are remarks on the patches, feel free to jump in.
Comment 26 Chris Leary [:cdleary] (not checking bugmail) 2012-02-05 21:17:44 PST
Comment on attachment 593054 [details] [diff] [review]
v4

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

I took a crack at this review, and I'll ask dvander to give it a skim as well to see if I've missed anything. r=me with comments addressed. Based on comment 25 if dvander thinks the review comments look okay I'll just modify them and put up a revised patch for him to review before we commit (with author as kosver, of course).

::: js/src/ion/CodeGenerator.cpp
@@ +841,5 @@
> +    return callVM(FValueToNotBoolean, ins);
> +}
> +
> +bool
> +CodeGeneratorX86Shared::visitCompareD(LCompareD *comp)

See later comment about this.

::: js/src/ion/LIR-Common.h
@@ +568,5 @@
> +    }
> +};
> +
> +// Not operation on a value
> +class LNotV : public LInstructionHelper<1, 1, 0>

I think that this should inherit from LCallInstructionHelper<1, BOX_PIECES, 0> since you'll perform a VM call and it acts on a Value.

::: js/src/ion/Lowering.cpp
@@ +854,5 @@
> +    //- double: LCompare(x, 0)
> +    //- null or undefined: true
> +    //- object: false 
> +    switch (op->type()) {
> +      

Nit: I don't think our style uses newline between cases or at the start of switch statments, and we generally only put braces around a switch case when you're declaring stuff inside it.

@@ +876,5 @@
> +      case MIRType_Object: 
> +        return define(new LInteger(0), ins);
> +
> +      case MIRType_Value: {
> +        LNotV *lir = new LNotV(useRegister(op)); 

I think this wants to be something like:

LNotV *lir = new LNotV;
if (!useBox(lir, LNotV::Input, op))
    return false;

::: js/src/ion/MIR.cpp
@@ +809,5 @@
> +{
> +    // Fold if the input is constant
> +    if (operand()->isConstant()) {
> +       const Value &v = operand()->toConstant()->value();
> +        /* ValueToBoolean can cause no side-effects, so this is safe */

Nit: // comments please.

::: js/src/ion/TypePolicy.h
@@ +73,5 @@
>      virtual bool adjustInputs(MInstruction *def);
>  };
>  
> +// Will always unbox the operands with a given specialization
> +class TryUnboxPolicy : public TypePolicy

I don't think we want this -- you'll choose the correct specialization during lowering based on the type of the MIR input definition.

::: js/src/ion/VMFunctions.cpp
@@ +140,5 @@
>      return true;
>  }
>  
> +bool
> +ValueToNotBoolean(JSContext *cx, const Value &input, JSBool *output)

s/ToNotBoolean/ToBooleanComplement

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ -260,4 @@
>  }
>  
>  bool
> -CodeGeneratorX86Shared::visitCompareD(LCompareD *comp)

I don't understand why this was moved into the other file. Is that just a mistake? If not, it still happens to have the CodeGeneratorX86Shared:: class prefix.

@@ +272,5 @@
> +{
> +    FloatRegister opd = ToFloatRegister(ins->input());
> +
> +    masm.xorpd(ScratchFloatReg, ScratchFloatReg);
> +    masm.compareDoubles(JSOP_EQ, opd, ScratchFloatReg);

Does this catch -0 (which starts with 0x8)? If so, please add a comment as to why.
Comment 27 Paul Wright 2012-02-11 07:54:25 PST
What is left to do here?
Comment 28 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-13 10:19:43 PST
(In reply to Chris Leary [:cdleary] from comment #26)
> ::: js/src/ion/Lowering.cpp
> @@ +854,5 @@
> > +    //- double: LCompare(x, 0)
> > +    //- null or undefined: true
> > +    //- object: false 
> > +    switch (op->type()) {
> > +      
> 
> Nit: I don't think our style uses newline between cases or at the start of
> switch statments, and we generally only put braces around a switch case when
> you're declaring stuff inside it.

At the start, definitely not.  Between cases, I think we're not consistent one way or the other.  I find lack of space between cases makes it harder to read the code, myself, because fallthrough instances are no longer distinguishable from non-fallthrough cases.
Comment 29 Chris Leary [:cdleary] (not checking bugmail) 2012-02-13 11:17:01 PST
With the rebased patch we incur 6 new failures, I'm investigating today.
Comment 30 Chris Leary [:cdleary] (not checking bugmail) 2012-02-13 17:27:10 PST
Got it working after a little TLC. https://hg.mozilla.org/projects/ionmonkey/rev/1a9f6de629e0

Let's make new bugs for the other patches if we want to get those in as well.

Note You need to log in before you can comment on or make changes to this bug.