Closed Bug 1021739 Opened 6 years ago Closed 5 years ago

Transform the selfhosting IsObject function into an intrinsic (cpp call)

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: h4writer, Assigned: connermcconkey, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(2 files, 6 obsolete files)

Currently we selfhost "IsObject" in JS (js/src/builtin/Utilities.js:146):

> /** Returns true iff Type(v) is Object; see ES5 8.6. */
> function IsObject(v) {
>     // Watch out for |typeof null === "object"| as the most obvious pitfall.
>     // But also be careful of SpiderMonkey's objects that emulate undefined
>     // (i.e. |document.all|), which have bogus |typeof| behavior.  Detect
>     // these objects using strict equality, which said bogosity doesn't affect.
>     return (typeof v === "object" && v !== null) ||
>            typeof v === "function" ||
>            (typeof v === "undefined" && v !== undefined);
> }     
    
This is quite nice and nothing is wrong with that.

But it is often used as following:
> if (IsObject(foo)) {
>    // foo is an object
> } else {
>    // foo is something else
> }

Where we assume that in the branch we will always have an object. Or in the else part we won't see an object. Currently IonMonkey isn't smart enough to improve the type information at the branch. It will assume all types flow in the if and else statement. There has been a start to fix this: bug 953164, which is currently still very limited.

In order to make the analysis easier so we don't have to learn IonMonkey all the compares in this function. We want to be able to match the function IsObject in IonMonkey easilly and replace it with one instruction: MIsObject. That would ease the implementation.

This bug consist of:
Part 1:
1) Create a cpp function in vm/SelfHosting.cpp intrinsic_IsObject which would do the same as the javascript function.
2) Remove the JS variant.
Part 2:
1) Create MIsObject / LIsObject / Codegeneration of IsObject
2) Adjust jit/MCallOptimize.cpp to match the "intrinsic_IsObject" function and replace it with MIsObject.

The actual improving of types at the branches is for a follow-up bug. Which I'm also willing to mentor if somebody is interested.

When there are questions or if you need help. You can contact me on irc in #ionmonkey or #jsapi
Whiteboard: [good first bug][mentor=h4writer][lang=c++]
Hello, I would be interested in taking this on as my first bug and having you mentor me. The first part of the bug seems simple, writing the cpp function as you explained, and is something I could get a start on. I don't quite understand the second part, but I still need to go through MCallOptimize.cpp, and get a handle on that. Thanks, I hope I am able to make a contribution.
Hi, Conner - 

If you've got your development environment spun up, then jump right into it. As soon as you have a first patch ready to show off, we can assign this bug to you.
(In reply to Conner McConkey from comment #1)
> Hello, I would be interested in taking this on as my first bug and having
> you mentor me. The first part of the bug seems simple, writing the cpp
> function as you explained, and is something I could get a start on. I don't
> quite understand the second part, but I still need to go through
> MCallOptimize.cpp, and get a handle on that. Thanks, I hope I am able to
> make a contribution.

Awesome. The first steps are indeed to get a development environment up. Getting the source from mozilla-central and creating a js shell build. (No need to fully build the browser).
Great to hear back from both of you. I've got my development environment set up and have been looking around the code. I hope to spend some time over the weekend to get a better grasp of the code related to the bug, and hopefully come up with a first patch in the near future. Good to know about the js shell build, as my computer takes a long time to build the full browser.
(In reply to Conner McConkey from comment #4)
> Great to hear back from both of you. I've got my development environment set
> up and have been looking around the code. I hope to spend some time over the
> weekend to get a better grasp of the code related to the bug, and hopefully
> come up with a first patch in the near future. Good to know about the js
> shell build, as my computer takes a long time to build the full browser.

Yeah that is the main reason for having such a shell. It takes a fraction of the time to build ;).

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
(the developer debug build)
Attached patch first patch for Bug 1021739 (obsolete) — Splinter Review
First patch for transforming the selfhosting IsObject function into an intrinsic
---
I have replaced the js IsObject function with a cpp function called intrinsic_IsObject, as well as, created a MIsObject. 

Changes:

*removed IsObject JS function from js/src/builtin/Utilities.js
*added intrinsic_IsObject function to js/src/vm/SelfHosting.cpp
*added visitIsObject function to js/src/jit/CodeGenerator.cpp
*added visitIsObject function to js/src/jit/Lowering.cpp
*added inlineIsObject function to js/src/jit/MCallOptimize.cpp
*added MIsObject class to /js/src/jit/MIR.h
*added declarations to various header files of file listed above

Tests:
Passed all jit-tests
Failed jstests with the following

REGRESSIONS
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/extensions/toLocaleFormat-02.js
    js1_8_5/extensions/findReferences-02.js
FAIL

-note: I believe the toLocaleFormat-01.js fails are due to different time settings on my Mac, and therefore not much of a problem. I will try to fix this soon. I'm still looking at findReferences-02.js and what means.

This is my first patch so I am quite unsure on how to structure this. Please let me know if I need to add any addition information. Thanks --- Conner
Comment on attachment 8437447 [details] [diff] [review]
first patch for Bug 1021739

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

This looks quite good for a first patch.
- There are some unnecessary whitespaces after lines. Esp. in MIR.h. If you press "review" you will see them highlighted in red.
- IsObject should work for any type. You made it only work for MIRType_Object. So I highlighted the things you'll need to change for that.

Thanks!

::: js/src/jit/CodeGenerator.cpp
@@ +8598,5 @@
>  }
> +    
> +bool
> +CodeGenerator::visitIsObject(LIsObject *ins)
> +{

The code here is actually much simpler. You need to check if a ValueOperand is an object:

Use testObject() and emitSet().

Please create a "testObjectSet" function that does that (like testNullSet does).

::: js/src/jit/LIR-Common.h
@@ +5756,5 @@
>          return mir_->toIsCallable();
>      }
>  };
>  
> +class LIsObject : public LInstructionHelper<1, 1, 0>

<BOX_PIECES, 1, 0>

(since on x86 we need 2 registers for a MIRType_value and 1 register for x64. Box_pieces contains the correct number for that)

::: js/src/jit/Lowering.cpp
@@ +3406,5 @@
>  
>  bool
> +LIRGenerator::visitIsObject(MIsObject *ins)
> +{
> +    JS_ASSERT(ins->object()->type() == MIRType_Object);

This assert isn't 'correct'

@@ +3408,5 @@
> +LIRGenerator::visitIsObject(MIsObject *ins)
> +{
> +    JS_ASSERT(ins->object()->type() == MIRType_Object);
> +    JS_ASSERT(ins->type() == MIRType_Boolean);
> +    return define(new(alloc()) LIsObject(useRegister(ins->object())), ins);

You will need to change this a bit. Since we want an MIRType_Value as input.
You can look at "visitTypeOf" for this.

::: js/src/jit/MCallOptimize.cpp
@@ +1886,5 @@
> +{
> +    if (callInfo.argc() != 1 || callInfo.constructing())
> +        return InliningStatus_NotInlined;
> +    if (getInlineReturnType() != MIRType_Object)
> +        return InliningStatus_NotInlined;

I think this is a typo. This should be MIRType_Boolean, right?

@@ +1888,5 @@
> +        return InliningStatus_NotInlined;
> +    if (getInlineReturnType() != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +    if (callInfo.getArg(0)->type() != MIRType_Object)
> +        return InliningStatus_NotInlined;

We want any type to flow any. So this check can get removed.

::: js/src/jit/MIR.h
@@ +9973,5 @@
>  };
>  
> +class MIsObject
> +    : public MUnaryInstruction,
> +    public SingleObjectPolicy

We want any type to flow in. So use a BoxInputsPolicy here. This will box the input to MIRType_Value.
(In reply to Conner McConkey from comment #6)
> Tests:
> Passed all jit-tests
> Failed jstests with the following
> 
> REGRESSIONS
>     js1_5/extensions/toLocaleFormat-01.js
>     js1_5/extensions/toLocaleFormat-02.js
>     js1_8_5/extensions/findReferences-02.js
> FAIL
> 
> -note: I believe the toLocaleFormat-01.js fails are due to different time
> settings on my Mac, and therefore not much of a problem. I will try to fix
> this soon. I'm still looking at findReferences-02.js and what means.
> 
> This is my first patch so I am quite unsure on how to structure this. Please
> let me know if I need to add any addition information. Thanks --- Conner

To be sure you should check if you have these errors without this patch too. I think they are indeed not related to the changes you made.
Blocks: 1023154
Assignee: nobody → connermcconkey
Attached patch bug1021739patch2.patch (obsolete) — Splinter Review
Hello, I'm back with my second patch. I've made the changes you outlined to the best of my ability. The whitespace has been improved and I remove the javascript function rather than just commenting it out. I created a testObjectSet function and use it in Codegenerator.cpp now, although I am not 100% sure if I am using ValueOperand correctly. I made the changes to Lowering.cpp as recommended and they made sense. But I am unsure if I am checking LIsObject correctly. Other than these two concerns the changes went well and I am understanding the JIT/JS code much better.
Attachment #8437447 - Attachment is obsolete: true
Comment on attachment 8439314 [details] [diff] [review]
bug1021739patch2.patch

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

With these changes altered you should try and run our test harness and see if everything still works:

in js/src/
python jit-test/jit-test.py --ion objdir/dist/bin/js

(This should be done with a optimized-debug build. So --enable-optimize --enable-debug as configure flags)

::: js/src/jit/LIR-Common.h
@@ +5778,5 @@
> +    public:
> +        LIR_HEADER(IsObject);
> +        explicit LIsObject(const LAllocation &object) {
> +            setOperand(0, object);
> +        }

Since the input is a Value now, we don't need to do setOperand here. That happens in Lowering.cpp with "useBox(lir, LIsObject::Input, opd)".

So this constructor can get removed

@@ +5785,5 @@
> +            return getTemp(0);
> +        }
> +        const LAllocation *object() {
> +            return getOperand(0);
> +        }

function temp() and object() are never used. So can get removed.

::: js/src/jit/Lowering.cpp
@@ +3414,5 @@
> +LIRGenerator::visitIsObject(MIsObject *ins)
> +{
> +    MDefinition *opd = ins->input();
> +    JS_ASSERT(opd->type() == MIRType_Value);
> +    LAllocation argsObj = useRegister(ins->input());

This line should get removed. (We have the useBox a few lines below).

::: js/src/jit/MCallOptimize.cpp
@@ +1911,5 @@
>  }
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineIsObject(CallInfo &callInfo)
> +{

Hehe. You did the requested changes to the wrong function :P
You did the changes to inlineToObject instead of this inlineIsObject.
So please restore the change to inlineToObject and do them for inlineIsObject.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +2301,5 @@
> +void
> +MacroAssemblerMIPSCompat::testObjectSet(Condition cond, const ValueOperand &value, Register dest)
> +{
> +    MOZ_ASSERT(cond == Equal || cond == NotEqual);
> +    ma_cmp_set(dest, value.typeReg(), ImmType(JSVAL_TYPE_NULL), cond);

You should update JSVAL_TYPE_NULL to be the correct thing ;)
(In reply to Conner McConkey from comment #9)
> Created attachment 8439314 [details] [diff] [review]
> bug1021739patch2.patch
> 
> Hello, I'm back with my second patch. I've made the changes you outlined to
> the best of my ability. The whitespace has been improved and I remove the
> javascript function rather than just commenting it out. I created a
> testObjectSet function and use it in Codegenerator.cpp now, although I am
> not 100% sure if I am using ValueOperand correctly. I made the changes to
> Lowering.cpp as recommended and they made sense. But I am unsure if I am
> checking LIsObject correctly. Other than these two concerns the changes went
> well and I am understanding the JIT/JS code much better.

Looks good. There are still some whitespaces left. The ValueOperand thing is done correctly but if you forgot to remove some old code and the codegenerator code now looks correct to me.

Thanks for doing this. Really appreciated.
If you have any questions feel free to mail me or come online on irc in #jsapi or #ionmonkey.
Attached patch patch 3 (obsolete) — Splinter Review
Hello, here's my third patch for the bug. I made all the changes except one that I did not understand; the change in MacroAssembler-mips.cpp. 

Also, how should I manage my changes in regards to hg? Today when I updated, hg said I needed to merge or force an update, so I merged but have not committed anything. I know how to commit then push using hg etc., but unsure when to do this. I assume that I should just make sure my local repo is up to date and keep all my changes local until we are ready to commit and push to the central repo.

Thanks, 
Conner
Attached patch bug1021739.patch (obsolete) — Splinter Review
Here is an up to date patch. I was learning to use mercuial queues and made a mess of my local repository, so I updated and then imported the patches and made the necessary changes. So now I have cleaner local repo and better handle on managing my queue.
Attachment #8439314 - Attachment is obsolete: true
Attachment #8440802 - Attachment is obsolete: true
Attachment #8440900 - Flags: review+
Comment on attachment 8440900 [details] [diff] [review]
bug1021739.patch

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

Oops, I actually meant to put review ? to my name. Not already r+ it. That's my task ;)
Attachment #8440900 - Flags: review+ → review?(hv1989)
Yeah, Sorry. I wasn't sure about that when I was filling it out. And then I got an email from bugzilla-daemon@mozilla.org so I figured I messed that up. Sorry if that causes you any extra work.
(In reply to Conner McConkey from comment #12)
> Also, how should I manage my changes in regards to hg? Today when I updated,
> hg said I needed to merge or force an update, so I merged but have not
> committed anything. I know how to commit then push using hg etc., but unsure
> when to do this. I assume that I should just make sure my local repo is up
> to date and keep all my changes local until we are ready to commit and push
> to the central repo.

You should use Mercurial Queues: https://developer.mozilla.org/en-US/docs/Mercurial_Queues

(Ignore what the big red warning box says: most developers working on mozilla-central use queues, so you're most likely to get help with using them and there are pretty good tools for them.)

The easiest way to get a pretty good Mercurial setup going is by running the unsurprisingly-named command `./mach mercurial-setup`. Documentation for that can be found here: https://developer.mozilla.org/en-US/docs/Installing_Mercurial
Comment on attachment 8440900 [details] [diff] [review]
bug1021739.patch

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

Looks good. Almost all comments are related to code style. So don't be thrown off by the big comment. That is normal to make these faults. They are actually mostly just whitespace you added.
If you addressed review comments, you can reupload the attachment and flag me for review again.

::: js/src/jit/CodeGenerator.cpp
@@ +8618,5 @@
>      masm.bind(&done);
>  
>      return true;
>  }
> +    

Style nit: remove the added whitespace

::: js/src/jit/LIR-Common.h
@@ +5758,5 @@
>  };
>  
>  class LIsCallable : public LInstructionHelper<1, 1, 0>
>  {
> +    public:

Style nit: Remove the indentation back to 2 spaces

@@ +5778,5 @@
> +    public:
> +        LIR_HEADER(IsObject);
> +        static const size_t Input = 0;
> +        MIsObject *mir() const {
> +            return mir_->toIsObject();

Fix the indentation of all these:

- "public:" should be on 2 spaces.
- "return ..." should be on 8 spaces.
- Everything else on 4 spaces

::: js/src/jit/Lowering.cpp
@@ +3428,5 @@
> +{
> +    MDefinition *opd = ins->input();
> +    JS_ASSERT(opd->type() == MIRType_Value);
> +    LIsObject *lir = new(alloc()) LIsObject();
> +    if (!useBox(lir, LIsObject::Input, opd))

You can use "useBoxAtStart" here. 

(This will give a hint to the compiler that input registers may overlap the output register. Which can decrease register pressure)

::: js/src/jit/MCallOptimize.cpp
@@ +1925,5 @@
> +    current->push(isObject);
> +    
> +    return InliningStatus_Inlined;
> +}
> +    

Style nit: remove all unneeded whitespaces.

@@ +1931,1 @@
>  IonBuilder::inlineToObject(CallInfo &callInfo)

The inlineToObject is not fully reverted to the old code. There is a comment gone and a whitespace added.

::: js/src/jit/MIR.h
@@ +10038,5 @@
>  
>      virtual void dump(FILE *fp) const;
>      virtual void dump() const;
>  };
> +    

Style nit: whitespace

@@ +10067,5 @@
> +    }
> +};
> +
> +class MIsObject
> +    : public MUnaryInstruction,

Style nit: only 2 spaces needed before ":"

@@ +10120,5 @@
>      AliasSet getAliasSet() const {
>          return AliasSet::None();
>      }
>  };
> +    

Style nit: whitespace

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1491,5 @@
>      void testNullSet(Condition cond, const ValueOperand &value, Register dest) {
>          cond = testNull(cond, value);
>          emitSet(cond, dest);
>      }
> +    

Style nit: whitespace

@@ +1496,5 @@
> +    void testObjectSet(Condition cond, const ValueOperand &value, Register dest) {
> +        cond = testObject(cond, value);
> +        emitSet(cond, dest);
> +    }
> +    

Style nit: whitespace

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +999,5 @@
>      void testNullSet(Condition cond, const ValueOperand &value, Register dest) {
>          cond = testNull(cond, value);
>          emitSet(cond, dest);
>      }
> +    

Style nit: whitespace

@@ +1004,5 @@
> +    void testObjectSet(Condition cond, const ValueOperand &value, Register dest) {
> +        cond = testObject(cond, value);
> +        emitSet(cond, dest);
> +    }
> +    

Style nit: whitespace

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +472,5 @@
>      void testNullSet(Condition cond, const ValueOperand &value, Register dest) {
>          cond = testNull(cond, value);
>          emitSet(cond, dest);
>      }
> +    

Style nit: whitespace

@@ +477,5 @@
> +    void testObjectSet(Condition cond, const ValueOperand &value, Register dest) {
> +        cond = testObject(cond, value);
> +        emitSet(cond, dest);
> +    }
> +    

Style nit: whitespace
Attachment #8440900 - Flags: review?(hv1989)
Mentor: hv1989
Whiteboard: [good first bug][mentor=h4writer][lang=c++] → [good first bug][lang=c++]
Attached patch bug1021739.patch (obsolete) — Splinter Review
Hey Hannes, 

I fixed those whitespaces. Let me know if there's anymore to do. I also will take a look at the follow up bug if I get the chance.

cheers,

Conner
Attachment #8440900 - Attachment is obsolete: true
Attachment #8442365 - Flags: review?(hv1989)
Attached patch bug1021739.patch (obsolete) — Splinter Review
Oops, just caught more whitespaces.
Attachment #8442365 - Attachment is obsolete: true
Attachment #8442365 - Flags: review?(hv1989)
Comment on attachment 8442371 [details] [diff] [review]
bug1021739.patch

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

Can you add "r=h4writer" on the subject/description of this patch.

::: js/src/jit/MCallOptimize.cpp
@@ +1916,5 @@
> +    if (callInfo.argc() != 1 || callInfo.constructing())
> +        return InliningStatus_NotInlined;
> +    if (getInlineReturnType() != MIRType_Boolean)
> +        return InliningStatus_NotInlined;
> +    

Style nit: I think this the last whitespace that shouldn't be here

::: js/src/vm/SelfHosting.cpp
@@ +63,5 @@
>      return true;
>  }
>  
>  bool
> +js::intrinsic_IsObject(JSContext *cx, unsigned argc, Value *vp){

Style nit: "{" should we on a newline
Attachment #8442371 - Flags: review+
Attachment #8442371 - Attachment is obsolete: true
Attachment #8442943 - Flags: review?(hv1989)
Comment on attachment 8442943 [details] [diff] [review]
bug1021739.patch r=h4writer

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

Can you compile this and run the jit-tests with it to see if there are no issues with this patch?

$ cd mozilla-central/js/src
$ python jit-test/jit-test.py --ion $JS_OBJ/dist/bin/js

::: js/src/jit/LIR-Common.h
@@ +5772,5 @@
>          return mir_->toIsCallable();
>      }
>  };
>  
> +class LIsObject : public LInstructionHelper<BOX_PIECES, 1, 0>

Oh. This should be:
<1, BOX_PIECES, 0>

as it stands for: <output registers, operand registers, temp registers>

::: js/src/vm/SelfHosting.cpp
@@ +64,5 @@
>  }
>  
>  bool
> +js::intrinsic_IsObject(JSContext *cx, unsigned argc, Value *vp)
> +{

You still miss something in SelfHosting.cpp. You need to declare the function as a intrinsic in SelfHosting.cpp:775
Attachment #8442943 - Flags: review?(hv1989)
Blocks: 1028910
Attached patch bug1021739.patchSplinter Review
Hello, I made those changes as you specified. Unfortunately there are errors with the jit tests so I'll be looking to fix those in the next couple days.

Here's the jit tests output:

>[ 684|   0|   0|   0]  16% ======>                                    | 214.0s
>FAIL - basic/array-tosource.js
>[2385|   1|   0|   0]  57% =======================>                   | 681.6s
>FAIL - debug/Frame-onStep-resumption-05.js
>[3220|   2|   0|   0]  77% ================================>          | 835.3s
>FAIL - ion/bug998059.js
>[3839|   3|   0|   0]  92% ======================================>    | 962.7s
>FAIL - latin1/date.js
>[3839|   4|   0|   0]  92% ======================================>    | 968.9s
>FAIL - latin1/index.js
>[3840|   5|   0|   0]  92% ======================================>    | 969.7s
>FAIL - latin1/json.js
>[3841|   6|   0|   0]  92% ======================================>    | 971.2s
>FAIL - latin1/search.js
>[3841|   7|   0|   0]  92% ======================================>    | 974.9s
>FAIL - latin1/regexp.js
>[3842|   8|   0|   0]  92% ======================================>    | 975.4s
>FAIL - latin1/quote.js
>[3842|   9|   0|   0]  92% ======================================>    | 975.7s
>FAIL - latin1/parseInt-parseFloat.js
>[3849|  10|   0|   0]  92% ======================================>    | 979.9s
>FAIL - latin1/toLowerCase-toUpperCase.js
>[3850|  11|   0|   0]  92% ======================================>    | 980.4s
>FAIL - latin1/trim.js
>[3851|  12|   0|   0]  92% ======================================>    | 981.2s
>FAIL - latin1/toNumber.js
>[3998|  13|   0|   0]  96% ========================================>  |1021.4s
>FAIL - proxy/bug897403.js
>[4162|  14|   0|   0] 100% ==========================================>|1055.2s
Attachment #8444561 - Flags: review?(hv1989)
(In reply to Conner McConkey from comment #23)
> Here's the jit tests output:

Can you run the jit tests without the patch applied. Jus to be sure you introduced the errors?
Any luck with this? When I last tested I saw no jit-test failures when running with the requested changes. So I really think it is caused by errors in the tree, which you based your patch on. That's why I asked to run jit-tests without the patch. If it still shows these errors. You didn't introduce them.
Flags: needinfo?(connermcconkey)
(In reply to Hannes Verschore [:h4writer] from comment #25)
> Any luck with this? When I last tested I saw no jit-test failures when
> running with the requested changes. So I really think it is caused by errors
> in the tree, which you based your patch on. That's why I asked to run
> jit-tests without the patch. If it still shows these errors. You didn't
> introduce them.

Hi Hannes. I've been busy with work last couple days, but I did get the chance to run the jit tests without my patch. The test had the exact same failures as when I ran mine, so they were not introduced by me :). Please let me know if there's anything else I can do. Also I'd be interested in starting to work on another bug if this one is almost complete.
Flags: needinfo?(connermcconkey)
(In reply to Conner McConkey from comment #26)
> (In reply to Hannes Verschore [:h4writer] from comment #25)
> > Any luck with this? When I last tested I saw no jit-test failures when
> > running with the requested changes. So I really think it is caused by errors
> > in the tree, which you based your patch on. That's why I asked to run
> > jit-tests without the patch. If it still shows these errors. You didn't
> > introduce them.
> 
> Hi Hannes. I've been busy with work last couple days, but I did get the
> chance to run the jit tests without my patch. The test had the exact same
> failures as when I ran mine, so they were not introduced by me :). Please
> let me know if there's anything else I can do. Also I'd be interested in
> starting to work on another bug if this one is almost complete.

Yes this seems complete. I'll review and upload Monday.

bug 1023154 is still free and is based on this work. It is about implementing LIsObjectAndBranch when a MIsObject follows a MTest. In that case we don't need to save the boolean into a register and let the test take that register to decide which branch to take. We can immediately branch.
Attachment #8444561 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/9854e43bfd1b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.