Closed
Bug 667132
Opened 14 years ago
Closed 14 years ago
IonMonkey: Fix greedy register allocation around loops
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(2 files, 2 obsolete files)
128 bytes,
text/plain
|
Details | |
29.93 KB,
patch
|
adrake
:
review+
|
Details | Diff | Splinter Review |
This patch is really two things:
(1) Fixes lots of bugs around phis in the greedy register allocator.
(2) Introduces a new LDefinition policy, called REDEFINED. adrake suggested this as a way to deal with two cases:
(a) box(constant-type, non-constant-data)
(b) unbox(constant->type, non-constant-data)
In both of these cases, we take in a register and basically do nothing. The allocator though sees a definition for these instructions and allocates a new register. "Redefinition" means that the definition specifies an existing virtual register, and the allocator ignores the definition completely. This required some rejiggering of the x86 code and I can't decide how ugly it is.
For the code: |var t; while (x) { t = t + 1; } return t;|
Before this patch:
B0:
5 parameter ([t:0 (arg -1)], [d:0 (arg -2)]) <|@
0 move ()[(arg:-2) -> (ebx)] <|@
7 box ([t:0 (edi)], [d:0 (esi)]) (c) <|@
9 goto (B1) <|@
B1:
0 move ()[(esi) -> (ecx)] <|@
12 unbox ([i:0 (ecx)]) (ecx), (edi) <|@
13 testvandbranch (B2, B3) (arg:-1), (ebx) <|@
B2:
0 move ()[(edi) -> (ecx)], [(esi) -> (edx)] <|@
14 return () (ecx), (edx) <|@
B3:
15 addi ([i:0 (ecx)]) (ecx), (c) <|@
16 box ([t:0 (ebx)], [d:0 (edx)]) (ecx) <|@
0 move ()[(ebx) -> (stack:i0)], [(edx) -> (stack:i1)] <|@
0 move ()[(stack:i0) -> (edi)], [(stack:i1) -> (esi)] <|@
18 goto (B1) <|@
After this patch:
B0:
5 parameter ([t (arg -1)], [d (arg -2)]) <|@
0 move ()[(arg:-2) -> (edi)] <|@
7 box ([t (ecx)], [d (esi)]) (c) <|@
9 goto () <|@
B1:
11 unbox ([i:11 (r)]) (ecx), (esi) <|@
12 testvandbranch (B2, B3) (arg:-1), (edi) <|@
B2:
0 move ()[(esi) -> (edx)] <|@
13 return () (ecx), (edx) <|@
B3:
14 addi ([i (esi)]) (esi), (c) <|@
15 box ([t (edi)], [d:15 (r)]) (esi) <|@
0 move ()[(edi) -> (ecx)], [(edi) -> (esi)] <|@
16 goto () <|@
Attachment #541885 -
Flags: review?(adrake)
![]() |
Assignee | |
Comment 1•14 years ago
|
||
Fixes two thinkos around virtual registers.
Attachment #541885 -
Attachment is obsolete: true
Attachment #542218 -
Flags: review?(adrake)
Attachment #541885 -
Flags: review?(adrake)
Comment 2•14 years ago
|
||
Comment on attachment 542218 [details] [diff] [review]
v2
Review of attachment 542218 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/LIR-Common.h
@@ +49,5 @@
>
> // Register spill/restore/resolve marker.
> class LMove : public LInstructionHelper<0, 0, 0>
> {
> + RegisterSet freeRegs;
Presumably this is so the code generator has access to a list of temporaries it can take advantage of?
I can generate information of this form during reification or resolution without too much difficulty. It feels kind of hacky, but I can't think of a better solution to that problem.
::: js/src/ion/MIR.h
@@ +330,5 @@
> void setEmitAtUses() {
> setFlags(EMIT_AT_USES);
> }
> + bool isEffectful() {
> + return hasFlags(EFFECTFUL);
This makes me want for the pseudo-monadic effect values Adobe's got...
::: js/src/ion/MIRGraph.h
@@ +411,4 @@
> MBasicBlock *loopSuccessor_;
>
> + // The predecessor block which is the backedge of a loop header.
> + MBasicBlock *backedge_;
Isn't this necessarily the predecessor of the loop successor block?
::: js/src/ion/x86/Lowering-x86.cpp
@@ +102,5 @@
> }
>
> LUnbox *lir = new LUnbox(unbox->type());
> + lir->setOperand(0, useType(inner));
> + lir->setOperand(1, usePayloadInRegister(inner));
No use of VREG_DATA_OFFSET and VREG_TYPE_OFFSET in this kind of case?
Comment 3•14 years ago
|
||
(In reply to comment #2)
> ::: js/src/ion/MIRGraph.h
> @@ +411,4 @@
> > MBasicBlock *loopSuccessor_;
> >
> > + // The predecessor block which is the backedge of a loop header.
> > + MBasicBlock *backedge_;
>
> Isn't this necessarily the predecessor of the loop successor block?
I've swung the other way on this one -- I actually want backedge in my register allocator, not loopSuccessor_ as the paper would have me believe. As I'm the only consumer of loopSuccessor_, want to just remove it while we're here?
Comment 4•14 years ago
|
||
I've rebased the patch in bug 657816 to make use of the backedge functionality.
Comment 5•14 years ago
|
||
This test case on at least x86_64 generates:
Assertion failure: def->policy() == LDefinition::PRESET, at /home/adrake/moz/ionmonkey/js/src/ion/GreedyAllocator.cpp:713
with this patch applied. This assertion does not trip with this patch not applied.
Comment 6•14 years ago
|
||
Comment on attachment 542218 [details] [diff] [review]
v2
Due to regression.
Attachment #542218 -
Flags: review?(adrake) → review-
![]() |
Assignee | |
Comment 7•14 years ago
|
||
After splitting critical edges the assert is gone. This is the same patch just rebased and removed stuff that shouldn't have been in there.
Attachment #542218 -
Attachment is obsolete: true
Attachment #544006 -
Flags: review?(adrake)
Comment 8•14 years ago
|
||
Comment on attachment 544006 [details] [diff] [review]
v3
Review of attachment 544006 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #544006 -
Flags: review?(adrake) → review+
![]() |
Assignee | |
Comment 9•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•