IonMonkey: Fuse BitAnd, Compare and Test into BitAndAndBranch

RESOLVED FIXED in Firefox 54

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sandervv, Assigned: sandervv)

Tracking

(Blocks: 1 bug, {perf})

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8827133 [details] [diff] [review]
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
(Assignee)

Updated

2 years ago
Assignee: nobody → sandervv
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8827133 - Flags: review?(jdemooij)
(Assignee)

Comment 1

2 years ago
The branch 'jne' in the example should be a 'je'.
Blocks: 1307062
Keywords: perf
Priority: -- → P3
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 <= ?
Attachment #8827133 - Flags: review?(jdemooij)
(Assignee)

Comment 3

2 years ago
Created attachment 8834535 [details] [diff] [review]
fuse-bitand-compare-test.patch

Added a test case and prevented optimizing inequality comparisons.
Attachment #8827133 - Attachment is obsolete: true
Attachment #8834535 - Flags: review?(jdemooij)
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 {
Attachment #8834535 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8836662 [details] [diff] [review]
fuse-bitand-compare-test.patch

Fixed nits.
Attachment #8834535 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a30525f7b395
Fuse BitAnd, Compare and Test into BitAndAndBranch r=jandem
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a30525f7b395
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.