Closed Bug 727480 Opened 9 years ago Closed 9 years ago

IonMonkey: Incorrect creation of def-use chains for the operands of the osrvalue and osrscopechain instructions

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: periclesroalves, Assigned: sstangl)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/534.52.7 (KHTML, like Gecko) Version/5.1.2 Safari/534.52.7

Steps to reproduce:

When running the following code, which is a simple division algorithm, IonMonkey produces the MIR graph that goes attached.
--
function div(a, b)                                                                                                                                     
{
        result = 0;

        if(b < a)
        {
            while(a >= b)
            {
                a -= b;
                result++;
            }
        }
        else return -1;

        return result;
}

div(100, 2);
--


Actual results:

Printing the number of uses of each instruction, we noticed that IonMonkey was not storing the uses of the osrentry (id=30) instruction.


Expected results:

As shown in the attached graph, the osrentry (id=30) instruction has four uses, one as an operand of the osrscopechain (id=32) instruction and other three as operands of the osrvalue (id=34, id=36 and id=38) instructions.

Further investigating this, we noticed a lack of a call to the initOperand function in the constructors of the MOsrValue and MOsrScopeChain classes, as done in other classes, like MStoreElement and others. 

When adding the missing calls, like in the following patch, it seems that things are now working just fine:


diff -r b860d2cd42f9 js/src/ion/MIR.h
--- a/js/src/ion/MIR.h	Mon Feb 06 15:21:47 2012 -0200
+++ b/js/src/ion/MIR.h	Mon Feb 06 15:30:06 2012 -0200
@@ -1957,6 +1957,7 @@
     MOsrValue(MOsrEntry *entry, ptrdiff_t frameOffset)
       : frameOffset_(frameOffset)
     {
+        initOperand(0,entry);
         setOperand(0, entry);
         setResultType(MIRType_Value);
     }
@@ -1987,6 +1988,7 @@
   private:
     MOsrScopeChain(MOsrEntry *entry)
     {
+        initOperand(0,entry);
         setOperand(0, entry);
         setResultType(MIRType_Object);
     }
Assignee: nobody → general
Component: General → JavaScript Engine
QA Contact: general → general
Great catch. This fixes the OSR instructions by having them extend MUnaryInstruction, which calls initOperand() in the constructor.

To make sure that Use information isn't missed again, this patch also extends AssertGraphCoherency() and calls it after each MIR-affecting phase. This caught a bug in LICM: remove() was being called with the intention to reposition an MInstruction, but remove() wipes out the Use information. Fixed by implementing a dedicated move instruction.
Attachment #600543 - Flags: review?(dvander)
Assignee: general → sstangl
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #600543 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/ebd61b600176
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.