Last Comment Bug 710983 - IonMonkey: LSRA does not spill correctly
: IonMonkey: LSRA does not spill correctly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: 695075 709731
  Show dependency treegraph
 
Reported: 2011-12-14 23:11 PST by David Anderson [:dvander]
Modified: 2011-12-27 15:14 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (251 bytes, application/x-javascript)
2011-12-14 23:11 PST, David Anderson [:dvander]
no flags Details
fix (16.00 KB, patch)
2011-12-15 02:00 PST, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Review
part 2: remove StackAssignment-$(ARCH) (33.24 KB, patch)
2011-12-16 17:33 PST, David Anderson [:dvander]
sstangl: review+
Details | Diff | Review
part 3: fix stack slot re-use bug (5.46 KB, patch)
2011-12-16 17:38 PST, David Anderson [:dvander]
sstangl: review+
Details | Diff | Review

Description David Anderson [:dvander] 2011-12-14 23:11:16 PST
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.
Comment 1 David Anderson [:dvander] 2011-12-14 23:11:53 PST
Created attachment 581891 [details]
test case
Comment 2 David Anderson [:dvander] 2011-12-14 23:30:11 PST
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.
Comment 3 David Anderson [:dvander] 2011-12-15 02:00:47 PST
Created attachment 581918 [details] [diff] [review]
fix

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 4 Jan de Mooij [:jandem] 2011-12-16 08:32:25 PST
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.
Comment 5 David Anderson [:dvander] 2011-12-16 17:33:20 PST
Created attachment 582443 [details] [diff] [review]
part 2: remove StackAssignment-$(ARCH)

 11 files changed, 159 insertions(+), 375 deletions(-)
Comment 6 David Anderson [:dvander] 2011-12-16 17:38:26 PST
Created attachment 582448 [details] [diff] [review]
part 3: fix stack slot re-use bug

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.
Comment 7 Jan de Mooij [:jandem] 2011-12-22 04:50:24 PST
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.
Comment 8 David Anderson [:dvander] 2011-12-27 15:14:48 PST
> 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

Note You need to log in before you can comment on or make changes to this bug.