Closed Bug 560578 Opened 15 years ago Closed 15 years ago

canRemat() and asm_restore() should agree

Categories

(Core Graveyard :: Nanojit, defect)

defect
Not set
normal

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
Whiteboard: PACMAN → PACMAN, fixed-in-nanojit
Whiteboard: PACMAN, fixed-in-nanojit → PACMAN, fixed-in-nanojit, fixed-in-tracemonkey
Assignee: edwsmith → nobody
Whiteboard: PACMAN, fixed-in-nanojit, fixed-in-tracemonkey → PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: