910960 - IonMonkey: Improve emitted code for guardtypeset

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Current situation for the checks in guardtypeset:
>  je matched
>  je matched
>  je matched
>  je matched
>  jump miss
> matched:

Improved to, by inverting the last check:
>  je matched
>  je matched
>  je matched
>  jne miss
> matched:

Improves codegen when there is only one test:
>  je matched
>  jump miss
> matched:

to (one jump less)

>  jne miss
> matched:

Improves richards with 7% locally. Didn't test the others.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug910960-guardtypeset

I know the original code was a bit better readable. I tried my best.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 797588 [details] [diff] [review]
bug910960-guardtypeset

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

::: js/src/jit/IonMacroAssembler.cpp
@@ +72,5 @@
>      JS_ASSERT(!types->unknown());
>  
> +    Label matched;
> +    types::Type tests[7] = {
> +        types::Type::Int32Type(),

Nice.

@@ +96,5 @@
> +    for (size_t i = 0; i < 7; i++) {
> +        if (types->hasType(tests[i])) {
> +            if (prev != types::Type::UnknownType())
> +                branchTestType(prev, Equal, tag, &matched);
> +            prev = tests[i];

Whoa, this is indeed hard to read, especially the bottom part with the manipulation of objects.

What we want to achieve here is to invert the last branch.  It sounds that one way to do it which is somehow visible under this code is to have a delayed code generation.  Such as we can still apply modification on the code before we spill everything down to the assembler.

We can have a new type, lets name it BranchTestTypeHunk.  This hunk will be constructed as if we were creating a branch in the macro assembler, except that instead of producing code, we produce a Hunk which will be used to produce code later on.  Then we can call an invert(Label *fail) method that we call before generating the jump.

for (…) {
  if (…) {
    masm.fromHunk(lastBranch);
    lastBranch = branchTestTypeHunk(***tests[i]***, Equal, tag, &matched);
  }
}

…

lastBranch.invert(&miss);
masm.fromHunk(lastBranch);
masm.bind(&matched);

Notice, that the default value of the hunk would be "never jump", and calling "invert(&miss)" would mean "always jump to miss", which handles nicely the case where the types set is empty.

I think this will clarify most of these as we would *feel like* generating everything at the right time. Just that we reserve the right to manipulate it later.  What do you think?

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: examplory solution

So you would be fine with the following? If I create a patch that would allow this?

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 800377 [details]
examplory solution

I think this is easier to read, even if Branch might be slightly more costly in terms of memory, as we have to save more info.


        lastBranch.invertCondition();
        lastBranch.relink(miss);

Interesting idea to have the relink as a separated method.  Do you have any other use case in mind where we can use that?

        if (!lastBranch.inited()) {

Prefer isInitialized()


    // Note: Some platforms give the same register for obj and scratch.
    // Make sure when writing to scratch, the obj register isn't used anymore!
    Label matched;

I guess the comment is probably miss-placed.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: bug910960-improve-codegen

This implements the needed code for the proposed solution.

(Note: this fails on /jit-test/tests/parallel/compare-values.js and jit-test/tests/parallel/ic-getelement.js in parallel compilation. I'll investigate this now. But there is nothing fundamentally wrong, so I expect it to be a small second patch to solve that.)

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 805963 [details] [diff] [review]
bug910960-improve-codegen

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

Nice work!

I cannot spot where the error might be, but you can r=me on this patch.

::: js/src/jit/IonMacroAssembler.h
@@ +53,5 @@
>              return masm_;
>          }
>      };
>  
> +    class Branch

I guess we can moved these classes to IonMacroAssembler.cpp in the mean time.

@@ +75,5 @@
> +            jump_(jump),
> +            reg_(reg)
> +        { }
> +
> +        bool isInitialized() {

nit: Add const on these accessors.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 805963 [details] [diff] [review]
bug910960-improve-codegen

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

::: js/src/jit/IonMacroAssembler.h
@@ +128,5 @@
> +                mirType = MIRType_Object;
> +            else
> +                MOZ_ASSUME_UNREACHABLE("Unknown conversion to mirtype");
> +
> +            masm.branchTestMIRType(cond(), reg(), mirType, jump());

Old code uses testNumber instead of testDouble for MIRType_Double. This is the failing compare-values.js test.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> ::: js/src/jit/IonMacroAssembler.h
> @@ +53,5 @@
> >              return masm_;
> >          }
> >      };
> >  
> > +    class Branch
> 
> I guess we can moved these classes to IonMacroAssembler.cpp in the mean time.

What is the benefit for this? And would this mean we can't use them in CodeGenerator.cpp? We don't have a use yet, but I don't want to exclude that...

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] (PTO:Sept 23-27) from comment #8)
> (In reply to Nicolas B. Pierron [:nbp] from comment #6)
> > ::: js/src/jit/IonMacroAssembler.h
> > @@ +53,5 @@
> > >              return masm_;
> > >          }
> > >      };
> > >  
> > > +    class Branch
> > 
> > I guess we can moved these classes to IonMacroAssembler.cpp in the mean time.
> 
> What is the benefit for this? And would this mean we can't use them in
> CodeGenerator.cpp? We don't have a use yet, but I don't want to exclude
> that...

The benefit is to scope it well where it is used, and avoid extra compilation time else-where.
If you think we can use it outside, then yes, it make sense to leave it in the header file.

Comment 10

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9db2450f2a16

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9db2450f2a16