709240 - IonMonkey: Compile JSOP_NOT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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]]

(In reply to Jan de Mooij (:jandem) from comment #0)
> - undefined/null: 0

Ehm 1 of course.

Comment 2

[Tom S [:evilpie]]

Attachment: WIP

not sure if i keep that design

Comment 3

[Tom S [:evilpie]]

Attachment: 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 S [:evilpie]]

Comment on attachment 586548 [details] [diff] [review]
v1

Looking at this, this is actually ready, sans handling of Value.

Comment 5

[kosver]

Attachment: 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 S [:evilpie]]

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]

Attachment: 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]

Who should review this?

Comment 9

[kosver]

Attachment: v2 part2: implement fastpath for double not

Added a fastpath for double not. Next on my list: not before compare/test

Comment 10

[kosver]

Attachment: v2 part2: implement fastpath for double not

fix bug with three not's in row

Comment 11

[kosver]

Attachment: v2 part3: remove not before compare and test

Comment 12

[kosver]

Attachment: 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] - inactive, e-mail if emergency]

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]

Attachment: v3

Comment 15

[kosver]

Attachment: v3 part2: implement fastpath for double not

Comment 16

[kosver]

Attachment: v3 part3: remove not before compare and test

Comment 17

[kosver]

Attachment: v3 part4: implement LCompareBAndBranch

Comment 18

[kosver]

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]]

Review ping. Not my patch but this blocks several SS/Kraken tests.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

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]

Attachment: 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]

Attachment: v4 part2: implement fastpath for double not

Comment 23

[kosver]

Attachment: v4 part3: remove not before compare and test

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

Bump! What can I do to help us get this opcode implementation in?

Comment 25

[kosver]

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)]

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]

What is left to do here?

Comment 28

[Jeff Walden [:Waldo]]

(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)]

With the rebased patch we incur 6 new failures, I'm investigating today.

Comment 30

[Chris Leary [:cdleary] (not checking bugmail)]

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.