Closed Bug 560578 Opened 10 years ago Closed 10 years ago

canRemat() and asm_restore() should agree

Categories

(Core Graveyard :: Nanojit, defect)

defect
Not set

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)

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
Blocks: 555255
Assignee: nobody → edwsmith
Status: NEW → ASSIGNED
Attachment #440609 - Flags: review?(nnethercote)
Target Milestone: --- → Future
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+
(In reply to comment #2)
>     // If it doesn't match asm_restore(), the register allocators decisions

Make that "register allocator's".
Whiteboard: PACMAN
NJ: http://hg.mozilla.org/projects/nanojit-central/rev/f4d4243db4cf
Whiteboard: PACMAN → PACMAN, fixed-in-nanojit
TM: http://hg.mozilla.org/tracemonkey/rev/7061e03640b8
Whiteboard: PACMAN, fixed-in-nanojit → PACMAN, fixed-in-nanojit, fixed-in-tracemonkey
TR: http://hg.mozilla.org/tamarin-redux/rev/a24af292fdf4
Assignee: edwsmith → nobody
Whiteboard: PACMAN, fixed-in-nanojit, fixed-in-tracemonkey → PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/7061e03640b8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.