Last Comment Bug 747271 - IonMonkey: Assertion failure: opd->type() == phi->type() on test_webgl_conformance.html
: IonMonkey: Assertion failure: opd->type() == phi->type() on test_webgl_confor...
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
: Jason Orendorff [:jorendorff]
: 737299 (view as bug list)
Depends on:
Blocks: IonGreen
  Show dependency treegraph
Reported: 2012-04-19 18:05 PDT by David Anderson [:dvander]
Modified: 2012-05-01 16:10 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

test case (636 bytes, application/javascript)
2012-04-19 18:16 PDT, David Anderson [:dvander]
no flags Details
fix (1.70 KB, patch)
2012-04-20 10:52 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description User image David Anderson [:dvander] 2012-04-19 18:05:41 PDT
Encountered this while trying to reproduce a separate bug.
Comment 1 User image David Anderson [:dvander] 2012-04-19 18:16:41 PDT
Created attachment 616834 [details]
test case
Comment 2 User image David Anderson [:dvander] 2012-04-20 10:52:55 PDT
Created attachment 617036 [details] [diff] [review]

One of the phi respecialization cases was just missing a reflow.
Comment 3 User image Nicolas B. Pierron [:nbp] 2012-04-20 15:41:35 PDT
Comment on attachment 617036 [details] [diff] [review]

Review of attachment 617036 [details] [diff] [review]:

Good, small refactoring nits & small perf issue.

::: js/src/ion/IonAnalysis.cpp
@@ +274,5 @@
>              // Specialize phis with int32 and double operands as double.
>              if (IsNumberType(use->type()) && IsNumberType(phi->type())) {
>                  use->specialize(MIRType_Double);
> +                if (!addPhiToWorklist(use))
> +                    return false;

It seems like you can factor addPhiToWorklist at the end of the loop by filtering the cases which do not need updates.

        // Same type, no need to update.
        if (use->type() == phi->type())

        // Propagate the most specialized type which fit both the use and the phi. 
        MIRType specialized = phi->type();
        if (use->type() != MIRType_None) {
            // Specialize phis with int32 and double operands as double.
            if (IsNumberType(use->type()) && IsNumberType(phi->type()))
                specialized = MIRType_Double;
                specialized = MIRType_Value;

        if (!addPhiToWorklist(use))
            return false;

There is one case which troubles me.  If the use type is already a Double, and the Phi is an Int32, then you re-specialize it and cause one extra useless iteration on the work list.  I suggest to check that the specialization is not current type.

        if (use->type() == specialize)
        if (!addPhiToWorklist(use))
            return false;
Comment 4 User image David Anderson [:dvander] 2012-04-26 09:28:48 PDT
pushed w/ nits fixed:
Comment 5 User image David Anderson [:dvander] 2012-05-01 16:10:23 PDT
*** Bug 737299 has been marked as a duplicate of this bug. ***

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