Closed
Bug 560578
Opened 15 years ago
Closed 15 years ago
canRemat() and asm_restore() should agree
Categories
(Core Graveyard :: Nanojit, defect)
Core Graveyard
Nanojit
Tracking
(Not tracked)
RESOLVED
FIXED
Future
People
(Reporter: edwsmith, Unassigned)
References
Details
(Whiteboard: PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)
Attachments
(1 file)
2.54 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
if canRemat() returns true for an instruction that asm_restore() cannot actually rematerialize, then it could cause suboptimal register allocation choices. asm_restore() will cause the instruction to spill anyway, when a better candidate might have been found for spilling at that point instead.
from code inspection, asm_restore and canRemat() disagree about some instructions:
ARM: immf
MIPS: immf
PPC: immf
PPC64: immf, immq
Sparc: immf
Reporter | ||
Comment 1•15 years ago
|
||
Reporter | ||
Updated•15 years ago
|
Target Milestone: --- → Future
![]() |
||
Comment 2•15 years ago
|
||
Comment on attachment 440609 [details] [diff] [review]
Make each backend's canRemat() agree with asm_restore()
>diff -r 54df30ab7ef3 nanojit/Assembler.h
>--- a/nanojit/Assembler.h Tue Apr 20 18:30:05 2010 -0700
>+++ b/nanojit/Assembler.h Wed Apr 21 17:03:13 2010 -0400
>@@ -348,7 +348,9 @@
> verbose_only(, size_t &nBytes));
>
> // These instructions don't have to be saved & reloaded to spill,
>- // they can just be recalculated cheaply.
>+ // they can just be recalculated cheaply. This function should return
>+ // true for the instructions that are handled explicitly without a spill
>+ // in asm_restore(), and false otherwise.
> static bool canRemat(LIns*);
>
> bool deprecated_isKnownReg(Register r) {
Can you make this "must match asm_restore()" part stronger? Eg:
// These instructions don't have to be saved & reloaded to spill,
// they can just be recalculated cheaply.
//
// WARNING: this function must match asm_restore() -- it should return
// true for the instructions that are handled explicitly without a spill
// in asm_restore(), and false otherwise.
//
// If it doesn't match asm_restore(), the register allocators decisions
// about which values to evict will be suboptimal.
And similarly put something like this above asm_restore()'s declaration in
Assembler.h: "WARNING: this must match canRemat()".
On i386 canRemat() doesn't match asm_restore() -- the latter handles some
LIR_paramp cases. It should either match, or have a comment explaining why
it doesn't.
r=me with these things fixed.
Attachment #440609 -
Flags: review?(nnethercote) → review+
![]() |
||
Comment 3•15 years ago
|
||
(In reply to comment #2)
> // If it doesn't match asm_restore(), the register allocators decisions
Make that "register allocator's".
Reporter | ||
Updated•15 years ago
|
Whiteboard: PACMAN
Reporter | ||
Comment 4•15 years ago
|
||
Whiteboard: PACMAN → PACMAN, fixed-in-nanojit
![]() |
||
Comment 5•15 years ago
|
||
Whiteboard: PACMAN, fixed-in-nanojit → PACMAN, fixed-in-nanojit, fixed-in-tracemonkey
Reporter | ||
Comment 6•15 years ago
|
||
Assignee: edwsmith → nobody
Whiteboard: PACMAN, fixed-in-nanojit, fixed-in-tracemonkey → PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Comment 7•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•11 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•