560578 - canRemat() and asm_restore() should agree

Status: RESOLVED FIXED

Description

[Edwin Smith]

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

Comment 1

[Edwin Smith]

Created attachment 440609 [details] [diff] [review]
Make each backend's canRemat() agree with asm_restore()

Comment 2

[Nicholas Nethercote [:njn]]

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.

Comment 3

[Nicholas Nethercote [:njn]]

(In reply to comment #2)
>     // If it doesn't match asm_restore(), the register allocators decisions

Make that "register allocator's".

Comment 4

[Edwin Smith]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/f4d4243db4cf

Comment 5

[Nicholas Nethercote [:njn]]

TM: http://hg.mozilla.org/tracemonkey/rev/7061e03640b8

Comment 6

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/a24af292fdf4

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7061e03640b8