Closed
Bug 1331350
Opened 7 years ago
Closed 7 years ago
IonMonkey: Fuse BitAnd, Compare and Test into BitAndAndBranch
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: sandervv, Assigned: sandervv)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
8.59 KB,
patch
|
Details | Diff | Splinter Review |
Fuse: # [BitOpI:bitand] andl $0x3, %esi # [CompareAndBranch:stricteq] testl %esi, %esi je .Lfrom1259 into: # [BitAndAndBranch] testb $0x3, %sil jne .Lfrom447
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sandervv
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Attachment #8827133 -
Flags: review?(jdemooij)
Assignee | ||
Comment 1•7 years ago
|
||
The branch 'jne' in the example should be a 'je'.
Updated•7 years ago
|
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Added a test case and prevented optimizing inequality comparisons.
Attachment #8827133 -
Attachment is obsolete: true
Attachment #8834535 -
Flags: review?(jdemooij)
Comment 4•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Keywords: checkin-needed
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a30525f7b395
Status: ASSIGNED → RESOLVED
Closed: 7 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.
Description
•