Changeset 275472 in webkit

Timestamp:

Apr 5, 2021 6:52:05 PM (3 years ago)

Author:

keith_miller@apple.com

Message:

DFG arity fixup nodes should exit to the caller's call opcode
​https://bugs.webkit.org/show_bug.cgi?id=223278

Reviewed by Saam Barati.

JSTests:

• stress/dfg-arity-fixup-uses-callers-exit-origin.js: Added.

(main.v22):
(main.v30):
(main.try.v40):
(main.try.v47):
(main.try.v56):
(main.):
(main):

Source/JavaScriptCore:

Right now when we do arity fixup in the DFG we model it in the
same way that it executes, which means all the nodes are part of
the callee. Unfortunately, this causes PhantomInsertionPhase to
think those nodes could be replacing previously defined
VirtualRegisters as they are part of the callee's header (always
alive). When PhantomInsertionPhase then inserts a Phantom it will
put that node in the caller's frame as that's the first ExitOK
node. The caller however may have no knowledge of that
VirtualRegister though. For example:

--> foo: loc10 is a local in foo.

...
1: MovHint(loc10)
2: SetLocal(loc10)

<-- foo loc10 ten is now out of scope for the InlineCallFrame of the caller.
...
Phantom will be inserted here refering to loc10, which doesn't make sense.
--> bar loc10 is an argument to bar and needs arity fixup.

... All of these nodes are ExitInvalid
3: MovHint(loc10, ExitInvalid)
4: SetLocal(loc10, ExitInvalid)
...

• dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::currentNodeOrigin):
(JSC::DFG::ByteCodeParser::inlineCall):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/dfg-arity-fixup-uses-callers-exit-origin.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (modified) (4 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2021-04-05  Keith Miller  <keith_miller@apple.com>
+
+        DFG arity fixup nodes should exit to the caller's call opcode
+        https://bugs.webkit.org/show_bug.cgi?id=223278
+
+        Reviewed by Saam Barati.
+
+        * stress/dfg-arity-fixup-uses-callers-exit-origin.js: Added.
+        (main.v22):
+        (main.v30):
+        (main.try.v40):
+        (main.try.v47):
+        (main.try.v56):
+        (main.):
+        (main):
+
 2021-04-02  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-04-05  Keith Miller  <keith_miller@apple.com>
+
+        DFG arity fixup nodes should exit to the caller's call opcode
+        https://bugs.webkit.org/show_bug.cgi?id=223278
+
+        Reviewed by Saam Barati.
+
+        Right now when we do arity fixup in the DFG we model it in the
+        same way that it executes, which means all the nodes are part of
+        the callee. Unfortunately, this causes PhantomInsertionPhase to
+        think those nodes could be replacing previously defined
+        VirtualRegisters as they are part of the callee's header (always
+        alive). When PhantomInsertionPhase then inserts a Phantom it will
+        put that node in the caller's frame as that's the first ExitOK
+        node. The caller however may have no knowledge of that
+        VirtualRegister though. For example:
+
+        --> foo: loc10 is a local in foo.
+            ...
+            1: MovHint(loc10)
+            2: SetLocal(loc10)
+        <-- foo // loc10 ten is now out of scope for the InlineCallFrame of the caller.
+        ...
+        // Phantom will be inserted here refering to loc10, which doesn't make sense.
+        --> bar // loc10 is an argument to bar and needs arity fixup.
+            ... // All of these nodes are ExitInvalid
+            3: MovHint(loc10, ExitInvalid)
+            4: SetLocal(loc10, ExitInvalid)
+            ...
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::currentNodeOrigin):
+        (JSC::DFG::ByteCodeParser::inlineCall):
+
 2021-04-02  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -750 +750 @@
     NodeOrigin currentNodeOrigin()
     {
-        CodeOrigin semantic;
-        CodeOrigin forExit;
-
-        if (m_currentSemanticOrigin.isSet())
-            semantic = m_currentSemanticOrigin;
-        else
-            semantic = currentCodeOrigin();
-
-        forExit = currentCodeOrigin();
+        CodeOrigin semantic = m_currentSemanticOrigin.isSet() ? m_currentSemanticOrigin : currentCodeOrigin();
+        CodeOrigin forExit = m_currentExitOrigin.isSet() ? m_currentExitOrigin : currentCodeOrigin();
 
         return NodeOrigin(semantic, forExit, m_exitOK);
@@ -1148 +1141 @@
     // The semantic origin of the current node if different from the current Index.
     CodeOrigin m_currentSemanticOrigin;
+    // The exit origin of the current node if different from the current Index.
+    CodeOrigin m_currentExitOrigin;
     // True if it's OK to OSR exit right now.
     bool m_exitOK { false };
@@ -1714 +1709 @@
         calleeVariable->mergeShouldNeverUnbox(true);
     }
+
+    // We want to claim the exit origin for the arity fixup nodes to be in the caller rather than the callee because
+    // otherwise phantom insertion phase will think the virtual registers in the callee's header have been alive from the last
+    // time they were set. For example:
+
+    // --> foo: loc10 is a local in foo.
+    //    ...
+    //    1: MovHint(loc10)
+    //    2: SetLocal(loc10)
+    // <-- foo: loc10 ten is now out of scope for the InlineCallFrame of the caller.
+    // ...
+    // --> bar: loc10 is an argument to bar and needs arity fixup.
+    //    ... // All of these nodes are ExitInvalid
+    //    3: MovHint(loc10, ExitInvalid)
+    //    4: SetLocal(loc10, ExitInvalid)
+    //    ...
+
+    // In this example phantom insertion phase will think @3 is always alive because it's in the header of bar. So,
+    // it will think we are about to kill the old value, as loc10 is in the header of bar and therefore always live, and
+    // thus need a Phantom. That Phantom, however, may be inserted  into the caller's NodeOrigin (all the nodes in bar
+    // before @3 are ExitInvalid), which doesn't know about loc10. If we move all of the arity fixup nodes into the
+    // caller's exit origin, forAllKilledOperands, which is how phantom insertion phase decides where phantoms are needed,
+    // will no longer say loc10 is always alive.
+    CodeOrigin oldExitOrigin = m_currentExitOrigin;
+    m_currentExitOrigin = currentCodeOrigin();
 
     InlineStackEntry* callerStackTop = m_inlineStackTop;
@@ -1817 +1837 @@
         // our callee's frame. We emit an ExitOK below.
     }
+
+    m_currentExitOrigin = oldExitOrigin;
 
     // At this point, it's again OK to OSR exit.