Closed Bug 677073 Opened 13 years ago Closed 13 years ago

IonMonkey: Double box generated during SSA construction

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached file Test case
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.
Attached patch fix (obsolete) — Splinter Review
Recent regression, which also caused snapshots to not get specialized inputs. I've added an assert to catch this for phis.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #551341 - Flags: review?(rpearl)
Attached patch better fix (obsolete) — Splinter Review
Attachment #551341 - Attachment is obsolete: true
Attachment #551341 - Flags: review?(rpearl)
Attachment #551350 - Flags: review?(rpearl)
Attached file Bad snapshot test case
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).
Attached patch fix v3Splinter Review
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)
Attachment #551367 - Attachment is obsolete: false
Attachment #551350 - Attachment is obsolete: true
Attachment #551350 - Flags: review?(rpearl)
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+
(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.