Closed
Bug 677073
Opened 13 years ago
Closed 13 years ago
IonMonkey: Double box generated during SSA construction
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: adrake, Assigned: dvander)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
In the attached test case, we end up with a phi with two inputs: one is a constant 0, the other is a copy of the parameter (not the unboxed data). The output of this phi is then boxed and returned, resulting in an incorrect function result (specifically, an integer value -127) on 32-bit.
Assignee | ||
Comment 1•13 years ago
|
||
Recent regression, which also caused snapshots to not get specialized inputs. I've added an assert to catch this for phis.
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #551341 -
Attachment is obsolete: true
Attachment #551341 -
Flags: review?(rpearl)
Attachment #551350 -
Flags: review?(rpearl)
Reporter | ||
Comment 3•13 years ago
|
||
With the current patch applied, snapshots can be produced with invalid virtual register uses (i.e. IDs larger than the number of virtual registers). This test case caused a crash in the linear scan allocator due to an out-of-bounds access into the virtual register list (just checked in http://hg.mozilla.org/projects/ionmonkey/rev/4ea28afe22cf which turned that into an assert).
Assignee | ||
Comment 4•13 years ago
|
||
Argh. Nice find. I'm not totally pleased with this fix but it's not too horrible. First, the hack around parameters is gone, parameters now just get the entry snapshots. Second, there's a hack for Phis instead. We could prevent this by giving phis snapshots, but that doesn't make a whole lot of sense, so I don't mind the two-line check.
Attachment #551367 -
Attachment is obsolete: true
Attachment #551377 -
Flags: review?(rpearl)
Assignee | ||
Updated•13 years ago
|
Attachment #551367 -
Attachment is obsolete: false
Assignee | ||
Updated•13 years ago
|
Attachment #551350 -
Attachment is obsolete: true
Attachment #551350 -
Flags: review?(rpearl)
Comment 5•13 years ago
|
||
Comment on attachment 551377 [details] [diff] [review] fix v3 Review of attachment 551377 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonAnalysis.cpp @@ +357,5 @@ > + return false; > + } else if (box->isPhi()) { > + if (box->block()->entrySnapshot() == snapshot) > + return false; > + } In terms of personal preference, I feel like it would be cleaner to fold all the returns together, something like: MSnapshot *defSnapshot; if (box->isInstruction()) defSnapshot = box->toInstruction()->snapshot(); else if (box->isPhi()) defSnapshot = box->block()->entrySnapshot(); return (snapshot == defSnapshot); I don't know whether that's house style though. @@ +367,5 @@ > + MDefinition *def = use->toDefinition(); > + > + // Only replace nodes that have a type policy. Otherwise, we would > + // replace an unbox into its own input. > + if (def->typePolicy()) phis don't have a typePolicy, but if someone unfamiliar with that reads this bit of code, it's a little unclear that phis will always fall through to the |if (def->isPhi()...| line. Maybe move lines 3750-376 above line 368. @@ +370,5 @@ > + // replace an unbox into its own input. > + if (def->typePolicy()) > + return true; > + > + // Except for phis, which if are specialized need specialized inputs. typo here: "if are specialized" => "if specialized" ::: js/src/jit-test/tests/ion/bug677073-2.js @@ +6,5 @@ > + v0 = v1 + v1; > + } > + return v0; > +} > +print(f0()); Comment expected output of the test, or at least "don't assert"? ::: js/src/jit-test/tests/ion/bug677073.js @@ +4,5 @@ > + x = p0; > + } > + return x; > +} > +print(a(1)); Likewise, comment expected output.
Attachment #551377 -
Flags: review?(rpearl) → review+
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Ryan Pearl [:rpearl] from comment #5) > MSnapshot *defSnapshot; > if (box->isInstruction()) > defSnapshot = box->toInstruction()->snapshot(); > else if (box->isPhi()) > defSnapshot = box->block()->entrySnapshot(); > > return (snapshot == defSnapshot); > > I don't know whether that's house style though. I like this better, too. http://hg.mozilla.org/projects/ionmonkey/rev/aad4f90fd9b5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•