canRemat() and asm_restore() should agree

RESOLVED FIXED in Future

Status

Core Graveyard
Nanojit
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Edwin Smith, Unassigned)

Tracking

unspecified
Future

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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)

Updated

8 years ago
Blocks: 555255
(Reporter)

Comment 1

8 years ago
Created attachment 440609 [details] [diff] [review]
Make each backend's canRemat() agree with asm_restore()
Assignee: nobody → edwsmith
Status: NEW → ASSIGNED
Attachment #440609 - Flags: review?(nnethercote)
(Reporter)

Updated

8 years ago
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".
(Reporter)

Updated

8 years ago
Whiteboard: PACMAN
(Reporter)

Comment 4

8 years ago
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
(Reporter)

Comment 6

8 years ago
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

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/7061e03640b8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.