IonMonkey: Compile JSOP_NOT

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: kosver)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 11 obsolete attachments)

4.90 KB, patch
Details | Diff | Splinter Review
v4
13.23 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
10.88 KB, patch
Details | Diff | Splinter Review
5.15 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

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

Ehm 1 of course.
Assignee: general → evilpies
Created attachment 586462 [details] [diff] [review]
WIP

not sure if i keep that design
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 on attachment 586548 [details] [diff] [review]
v1

Looking at this, this is actually ready, sans handling of Value.
Attachment #586548 - Attachment description: WIP v2 → v1
Attachment #586548 - Flags: review?(dvander)
(Assignee)

Comment 5

5 years ago
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
Attachment #586548 - Flags: review?(dvander)
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #586722 - Attachment is obsolete: true
Attachment #587306 - Flags: review?
Assignee: evilpies → nikosverschore

Comment 8

5 years ago
Who should review this?
(Assignee)

Updated

5 years ago
Attachment #587306 - Flags: review? → review?(dvander)
(Assignee)

Comment 9

5 years ago
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
Attachment #587485 - Flags: review?(dvander)
(Assignee)

Comment 10

5 years ago
Created attachment 587647 [details] [diff] [review]
v2 part2: implement fastpath for double not

fix bug with three not's in row
Attachment #587485 - Attachment is obsolete: true
Attachment #587485 - Flags: review?(dvander)
Attachment #587647 - Flags: review?(dvander)
(Assignee)

Comment 11

5 years ago
Created attachment 587703 [details] [diff] [review]
v2 part3: remove not before compare and test
Attachment #587703 - Flags: review?(dvander)
(Assignee)

Comment 12

5 years ago
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.
Attachment #587704 - Flags: review?(dvander)
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!
Attachment #587306 - Flags: review?(dvander)
(Assignee)

Comment 14

5 years ago
Created attachment 588641 [details] [diff] [review]
v3
Attachment #587306 - Attachment is obsolete: true
Attachment #588641 - Flags: review?(dvander)
(Assignee)

Comment 15

5 years ago
Created attachment 588642 [details] [diff] [review]
v3 part2: implement fastpath for double not
Attachment #587647 - Attachment is obsolete: true
Attachment #587647 - Flags: review?(dvander)
(Assignee)

Comment 16

5 years ago
Created attachment 588643 [details] [diff] [review]
v3 part3: remove not before compare and test
Attachment #587703 - Attachment is obsolete: true
Attachment #587703 - Flags: review?(dvander)
(Assignee)

Comment 17

5 years ago
Created attachment 588644 [details] [diff] [review]
v3 part4: implement LCompareBAndBranch
Attachment #587704 - Attachment is obsolete: true
Attachment #587704 - Flags: review?(dvander)
(Assignee)

Comment 18

5 years ago
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?
(Reporter)

Comment 19

5 years ago
Review ping. Not my patch but this blocks several SS/Kraken tests.
Status: NEW → ASSIGNED
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
Attachment #588641 - Flags: review?(dvander)
(Assignee)

Comment 21

5 years ago
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
Attachment #586462 - Attachment is obsolete: true
Attachment #586548 - Attachment is obsolete: true
Attachment #588641 - Attachment is obsolete: true
Attachment #593054 - Flags: review?(dvander)
(Assignee)

Comment 22

5 years ago
Created attachment 593060 [details] [diff] [review]
v4 part2: implement fastpath for double not
Attachment #588642 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Created attachment 593061 [details] [diff] [review]
v4 part3: remove not before compare and test
Attachment #588643 - Attachment is obsolete: true
Bump! What can I do to help us get this opcode implementation in?
(Assignee)

Comment 25

5 years ago
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 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.
Attachment #593054 - Flags: review?(dvander) → review+

Comment 27

5 years ago
What is left to do here?
(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.
With the rebased patch we incur 6 new failures, I'm investigating today.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.