Last Comment Bug 733966 - IonMonkey: Improve NotV and test(not(foo))
: IonMonkey: Improve NotV and test(not(foo))
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-07 17:08 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-03-16 16:04 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/betterNot-r1.patch (8.61 KB, patch)
2012-03-07 17:08 PST, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Review

Description Marty Rosenberg [:mjrosenb] 2012-03-07 17:08:21 PST
Created attachment 603920 [details] [diff] [review]
/home/mrosenberg/patches/betterNot-r1.patch

currently notV uses a vmcall to handle something that we (mostly) know how to handle currently.
I've replaced the vmcall with a call to the innards of what used to be visitTestVAndBranch, but rather than branching to other blocks, the two cases branch to a mov 0, dest and a mov 1, dest.

I've also changed MTest(MNot(x), ifTrue, ifFalse) to fold to MTest(x, ifFalse, ifTrue).

I First noticed the issue with v8-splay, so here are the v8 tests:

TEST              COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:      1.021x as fast    3786.2ms +/- 0.7%   3709.2ms +/- 0.9%     significant

=============================================================================

  v8:             1.021x as fast    3786.2ms +/- 0.7%   3709.2ms +/- 0.9%     significant
    crypto:       -                  110.7ms +/- 2.6%    110.7ms +/- 2.2% 
    deltablue:    1.028x as fast     638.6ms +/- 1.2%    621.5ms +/- 1.7%     significant
    earley-boyer: 1.020x as fast    1607.7ms +/- 1.1%   1575.7ms +/- 1.1%     significant
    raytrace:     1.026x as fast     776.0ms +/- 2.0%    756.6ms +/- 1.8%     significant
    regexp:       -                  396.8ms +/- 0.8%    393.5ms +/- 1.2% 
    richards:     -                  150.7ms +/- 1.8%    148.8ms +/- 1.3% 
    splay:        1.030x as fast     105.5ms +/- 1.9%    102.5ms +/- 1.6%     significant


There is also a small snippet #if 0'ing out a piece of debug code that exists in a patch that has already been applied locally.  It won't go into the repo.
Comment 1 Sean Stangl [:sstangl] 2012-03-13 15:57:31 PDT
Comment on attachment 603920 [details] [diff] [review]
/home/mrosenberg/patches/betterNot-r1.patch

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

r+ with changes

::: js/src/ion/CodeGenerator.cpp
@@ +192,3 @@
>  
> +bool
> +CodeGenerator::generateBranchV(const ValueOperand &value, Label *ifTrue, Label *ifFalse, FloatRegister fr)

The signature is reasonable, but the name violates some arbitrary internal conventions: we have been using "generate" to imply the creation of per-IonScript ASM shims; "branch" generally implies taking an Assembler::Condition argument.

We could use that convention to reorganize the code to better match the rest of the engine. Suppose that instead of generateBranchV(), we had the following signatures:

Condition testValue(Condition cond, const ValueOperand &val, const FloatRegister &tmpfr);
void branchTestValue(Condition cond, const ValueOperand &val, const FloatRegister &tmpfr);

Then we could very easily move this code to ion/IonMacroAssembler.h, and it would exactly match the design of the rest of the engine. Usage by the caller is mostly the same, just with the ifFalse case needing to be handled externally.

I think this would be a lot cleaner.

@@ +1230,5 @@
> +    Label join;
> +    if (!generateBranchV(ToValue(lir, LNotV::Input), &setFalse, &setTrue, ToFloatRegister(lir->tempFloat())))
> +        return false;
> +    masm.bind(&setTrue);
> +    masm.mov(Imm32(1), ToRegister(lir->getDef(0)));

Generic code uses |move32()|.

@@ +1231,5 @@
> +    if (!generateBranchV(ToValue(lir, LNotV::Input), &setFalse, &setTrue, ToFloatRegister(lir->tempFloat())))
> +        return false;
> +    masm.bind(&setTrue);
> +    masm.mov(Imm32(1), ToRegister(lir->getDef(0)));
> +    masm.jump(&join);

The true case should be the expected case, and should not require an extra jump. Please switch positioning of true/false blocks.

@@ +1233,5 @@
> +    masm.bind(&setTrue);
> +    masm.mov(Imm32(1), ToRegister(lir->getDef(0)));
> +    masm.jump(&join);
> +    masm.bind(&setFalse);
> +    masm.mov(Imm32(0), ToRegister(lir->getDef(0)));

Let's just use xor() and not count on the assembler to realize what should be emitted.

@@ +1235,5 @@
> +    masm.jump(&join);
> +    masm.bind(&setFalse);
> +    masm.mov(Imm32(0), ToRegister(lir->getDef(0)));
> +    masm.bind(&join);
> +    return true;

This code is very dense. As a personal preference, I think it's a bit clearer to put whitespace between blocks of code separated by jump targets. If those blocks are large, or are independent of the above code (without fallthrough), it also can look nice to put them in their own indented {} block.

For the sake of example, this would make the above code (without above changes):

{
    Label setTrue, setFalse, join;

    if (!generateBranchV(...))
        return false;

    masm.bind(&setTrue);
    masm.move(Imm32(1), ToRegister(lir->getDef(0)));
    masm.jump(&join);

    masm.bind(&setFalse);
    masm.move(Imm32(0), ToRegister(lir->getDef(0)));

    masm.bind(&join);
    return true;
}

::: js/src/ion/LIR-Common.h
@@ +769,1 @@
>  };

nit: Extra whitespace after previous }

::: js/src/ion/MIR.cpp
@@ +192,5 @@
> +    MDefinition *tmp = op->foldsTo(useValueNumbers);
> +
> +    if (tmp->isNot()) {
> +        return MTest::New(tmp->toNot()->operand(), ifFalse(), ifTrue());
> +    }

nit: {} unneeded for single-line if().

@@ +194,5 @@
> +    if (tmp->isNot()) {
> +        return MTest::New(tmp->toNot()->operand(), ifFalse(), ifTrue());
> +    }
> +    if (tmp != op) {
> +        return MTest::New(tmp, ifTrue(), ifFalse());

Is this really needed? If the operand folds to something, then that folding should already have occurred prior to the execution of the dependent MTest::foldsTo(); i.e., all operands should already be maximally folded.

Folding (via simplify()) occurs by RPO over blocks, with forward iteration through instructions per block.

@@ +195,5 @@
> +        return MTest::New(tmp->toNot()->operand(), ifFalse(), ifTrue());
> +    }
> +    if (tmp != op) {
> +        return MTest::New(tmp, ifTrue(), ifFalse());
> +    }

nit: {} unneeded
Comment 2 Marty Rosenberg [:mjrosenb] 2012-03-16 16:04:25 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/424c093bca95

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