1331350 - IonMonkey: Fuse BitAnd, Compare and Test into BitAndAndBranch

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Sander Mathijs van Veen]

Attachment: fuse-bitand-compare-test.patch

Fuse:

    # [BitOpI:bitand]
    andl       $0x3, %esi
    # [CompareAndBranch:stricteq]
    testl      %esi, %esi
    je         .Lfrom1259

into:

    # [BitAndAndBranch]
    testb      $0x3, %sil
    jne        .Lfrom447

Comment 1

[Sander Mathijs van Veen]

The branch 'jne' in the example should be a 'je'.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8827133 [details] [diff] [review]
fuse-bitand-compare-test.patch

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

Sorry for the delay! This looks good, but a question below and this definitely needs some tests. Both for the cases we will optimize and cases we don't optimize because they fail some of the new conditions.

::: js/src/jit/Lowering.cpp
@@ +796,5 @@
> +            if (lhs->type() == MIRType::Int32 && rhs->type() == MIRType::Int32) {
> +                ReorderCommutative(&lhs, &rhs, test);
> +                Assembler::Condition cond = JSOpToCondition(comp->compareType(),
> +                                                            comp->jsop());
> +                lowerForBitAndAndBranch(new(alloc()) LBitAndAndBranch(ifTrue, ifFalse, cond),

Hm the condition after a test should be Zero or NonZero right? It seems this patch will pass Equal/NotEqual, which is a bit confusing. Also, what will we do for operators like <= ?

Comment 3

[Sander Mathijs van Veen]

Attachment: fuse-bitand-compare-test.patch

Added a test case and prevented optimizing inequality comparisons.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8834535 [details] [diff] [review]
fuse-bitand-compare-test.patch

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

Looks good, thanks! Just some nits.

::: js/src/jit-test/tests/ion/bug1331350.js
@@ +13,5 @@
> +
> +    return a + b | 0
> +}
> +
> +assertEq(optimize(4 | 0, 6 | 0), 12);

Nit: add a for (var i=0; i<20; i++) loop around these calls, and below.

@@ +47,5 @@
> +
> +    return a + b | 0
> +}
> +
> +assertEq(not_optimizible(4 | 0, 6 | 0), 12);

Nit: s/optimizible/optimizable/

::: js/src/jit/Lowering.cpp
@@ +778,5 @@
>          add(new(alloc()) LGoto(ifTrue));
>          return;
>      }
>  
> +

Nit: remove second blank line.

@@ +780,5 @@
>      }
>  
> +
> +    if (opd->isCompare() && opd->isEmittedAtUses()) {
> +        MCompare* comp = opd->toCompare();

Nit: add a comment before this line, something like:

// Emit LBitAndBranch for cases like |if ((x & y) === 0)|.

@@ +788,5 @@
> +            comp->getOperand(1)->toConstant()->isInt32(0) &&
> +            comp->getOperand(0)->isBitAnd() &&
> +            comp->getOperand(0)->isEmittedAtUses())
> +        {
> +            MDefinition* ba = opd->getOperand(0);

Nit: I wondered for a second what ba meant, s/ba/bitAnd/ would be clearer.

@@ +801,5 @@
> +            {
> +                ReorderCommutative(&lhs, &rhs, test);
> +                if (cond == Assembler::Equal)
> +                    cond = Assembler::Zero;
> +                else if(cond == Assembler::NotEqual)

Nit: add space after if

::: js/src/jit/shared/LIR-shared.h
@@ +2964,1 @@
>      {

MOZ_ASSERT(cond == Assembler::Zero || cond == Assembler::NonZero);

@@ +2977,5 @@
>      }
>      const LAllocation* right() {
>          return getOperand(1);
>      }
> +    Assembler::Condition cond() {

Nit: can make this const:

cond() const {

Comment 5

[Sander Mathijs van Veen]

Attachment: fuse-bitand-compare-test.patch

Fixed nits.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a30525f7b395
Fuse BitAnd, Compare and Test into BitAndAndBranch r=jandem

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/a30525f7b395