597154 - Nanojit should not spill remat-able values

Status: RESOLVED WONTFIX

(Core Graveyard :: Nanojit, defect)

Description

[Steven Johnson]

We're currently spilling values that don't need to be spilled (eg, constants).

Comment 1

[Steven Johnson]

Attachment: Naive patch

Not sure if the fix is this simple?

Comment 2

[Steven Johnson]

As an example, every TR method emits at least two constants: 4 (undefinedAtom) and the AvmCore pointer. Both of these are remat-able but are spilled anyway.

Comment 3

[Edwin Smith]

Not sure.  The semantics of the isInAr() flag mean that if the flag is set, some downstream code is loading from that stack slot, therefore a value must be written there.

It's a bug if an instruction that has isInAr() = true doesn't store a value in that slot.  It's also a bug (although benign) if the flag is set when no instruction ever loads from that slot.  

The second case happens in known situations.  Any value that's defined before a loop, and live inside the loop, must have its live range extended across the whole loop.  Because of spilling, the assembler has to reserve a stack slot for the instruction.  However, a spill might not happen, and the reservation could be wasted.

The assembler doesn't distinguish beween stack-slot-reserved, and stack-slot-required.  Bill and I have talked about improving the AR logic to handle this.  In the mean time, the extra spills are a FOL.

Comment 4

[Edwin Smith]

Also I think the canRemat() result is a red herring; strictly speaking, its not illegal for a rematerizeable value to be spilled.  (I can safely say that, becuase illegal means "assert" and you aren't seeing an assert).

It might be the case that all the uses surrounding canRemat() could be tightened up -- namely, that canRemat() *guarantees* a value won't be spilled, and therefore we should be able to assert that if canRemat() is true, isInAr() is false.  but, audit is required.

Comment 5

[Steven Johnson]

Yeah, the above patch definitely isn't right. Definitely worth figuring out the right answer; emitting unnecessary stores is icky.

Comment 6

[Nicholas Nethercote [inactive]]

Nanojit has been dead for several years. Its Bugzilla component has been moved to the graveyard (bug 984276).

I checked all the open bugs. They're all uninteresting, so I'm WONTFIXing them all. Apologies for the bugspam.