Found this while researching LSRA's spill code for bug 695075. The attached test case fails with --ion-eager. The bug is that LSRA tries to optimize spills, so if you have: var a = ...; if (a) { print(a); } Then |a| will only be spilled on the branch. But if the spilled interval is split, the next interval can cover both branches. If *that* interval is split on the second branch, it will attempt to load from the canonical spill location which may not have been stored. The easy fix for this is to hoist stack stores to the point of definition. We can keep the existing optimization, I think, if there is only one spill.

Note that this testcase needs the queue in bug 695075 to repro - only because unboxes were changed to create new intervals, so unboxed arguments may now need to spill.

This patch changes where spills occur on certain instructions. If an instruction has multiple spill intervals, or is outside the loop its spill is in, the spill is will now occur immediately after the defining instruction. Otherwise, the old logic takes place. These changes greatly simplify the safepoint building process, since it's now easy to determine whether a non-spill interval is actually spilled. I'm going to let anion run on this overnight.

Comment on attachment 581918 [details] [diff] [review] fix Review of attachment 581918 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/LinearScan.cpp @@ +1026,5 @@ > + > + // If this spill is inside a loop, and the definition is outside > + // the loop, instead move the spill to outside the loop. > + VirtualRegister *other = &vregs[current->start()]; > + if (other->block()) { I don't think other->block() can be NULL, It looks like all callers of VirtualRegister::init pass in a non-NULL block. If we need the check, a comment would be useful. @@ +1030,5 @@ > + if (other->block()) { > + uint32 loopDepthAtDef = reg->block()->mir()->loopDepth(); > + uint32 loopDepthAtSpill = other->block()->mir()->loopDepth(); > + if (loopDepthAtSpill > loopDepthAtDef) > + reg->setSpillAtDefinition(); Nice! ::: js/src/ion/LinearScan.h @@ +333,5 @@ > Vector<LOperand, 0, IonAllocPolicy> uses_; > LMoveGroup *inputMoves_; > LMoveGroup *outputMoves_; > LAllocation *canonicalSpill_; > + bool spillAtDefinition_; VirtualRegisterMap::init() uses memset to set all fields to zero, but I think VirtualRegister's constructor should either initialize everything (including spillAtDefinition_) to false/NULL/etc or initialize none of the fields.

11 files changed, 159 insertions(+), 375 deletions(-)

Part 1 exposed another bug in LSRA. We can use and free a stack slot inside a loop, and then while still inside the loop, reserve the same slot for a virtual register that crosses the entire loop. This patch stops using the StackSlotAllocator's free list, in favor of a less aggressive version that only grabs a slot if the intervals don't overlap.

I spent some time today tracking down a jit-test failure with --ion-eager for bug 709731, but it turned out to be this bug. We'd spill inside a while(0) {} loop and assume the value was spilled after the loop.

> VirtualRegisterMap::init() uses memset to set all fields to zero, but I > think VirtualRegister's constructor should either initialize everything > (including spillAtDefinition_) to false/NULL/etc or initialize none of the > fields. Yeah, the constructor here doesn't make any sense. I'll remove it. http://hg.mozilla.org/projects/ionmonkey/rev/32b53718b18e http://hg.mozilla.org/projects/ionmonkey/rev/3057c1ecb258 http://hg.mozilla.org/projects/ionmonkey/rev/0c9e9f29fd85