PutStack sinking phase inserts PutStack with wrong value

https://bugs.webkit.org/show_bug.cgi?id=256872
rdar://109752832

Reviewed by Keith Miller.

Fixes a bug where the DFG PutStack sinking phase would put the wrong
value to the stack. This is due to the PutStack sinking phase doing its
own SSA calculation to determine the correct locations to insert
PutStacks, which is not necessarily the same as correctly tracking the
values that should belong in each PutStack (which is just SSA conversion).

This patch adds an additional PutStack node after each Phi inserted in
SSA conversion. As a result, when PutStack sinking interprets each block
to map each variable to its current value, it correctly associates
existing Phis with their variables, even if the PutStack sinking phase's
SSA calculation would not insert a Phi there.

One concern with this patch is performance - specifically, adding
additional PutStacks might pessimize arguments elimination. To address
this, this patch runs PutStack sinking both before and after arguments
elimination, so we reduce spurious PutStacking before eliminating
arguments, and re-run it after to exploit any opportunities created by
the existing phase ordering. Testing on both Speedometer 2 and JetStream
2, there is no significant change in score with this patch.

* JSTests/stress/PutStack-sinking-through-DeadFlush-unification.js: Added.
(test):
(main):
* Source/JavaScriptCore/dfg/DFGPlan.cpp:
(JSC::DFG::Plan::compileInThreadImpl):
* Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:
* Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:
(JSC::DFG::SSAConversionPhase::run):

Canonical link: https://commits.webkit.org/265866@main

JSTests/stress/PutStack-sinking-through-DeadFlush-unification.js

@@ -0,0 +1,30 @@
+function test(a, flag, flag2, bailout) {
+    a = 3333
+    if (flag) {
+        if (flag2) {
+            a = 1111;
+            Object.toString();
+        } else {
+            a = 2222;
+            Object.toString();
+        }
+        Object.toString();
+    }
+    Object.toString();
+    bailout ++;
+    return a;
+}
+
+function main() {
+    for(let i = 0; i < 1000; i ++) {
+        test(0, true, true, 0);
+        test(0, true, false, 0);
+        test(0, false, true, 0);
+        test(0, false, false, 0);
+    }
+
+    let result = test(0, true, true, 0x7fffffff);
+    if (result !== 1111)
+        throw 'Expected test() to return 1111, got ' + result;
+}
+main()

Source/JavaScriptCore/dfg/DFGPlan.cpp

@@ -372,6 +372,8 @@ Plan::CompilationPath Plan::compileInThreadImpl()
         RUN_PHASE(performSSALowering);
 
         // Ideally, these would be run to fixpoint with the object allocation sinking phase.
+        if (Options::usePutStackSinking())
+            RUN_PHASE(performPutStackSinking);
         RUN_PHASE(performArgumentsElimination);
         if (Options::usePutStackSinking())
             RUN_PHASE(performPutStackSinking);

Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp

@@ -504,8 +504,11 @@ class PutStackSinkingPhase : public Phase {
                         Node* incoming = mapping.operand(operand);
                         DFG_ASSERT(m_graph, node, incoming);
 
+                        // If we are sinking a PutStack to before an ExitOK, it
+                        // should be ExitInvalid like the other preceding nodes.
+                        auto origin = node->op() == ExitOK ? node->origin.withInvalidExit() : node->origin;
                         insertionSet.insertNode(
-                            nodeIndex, SpecNone, PutStack, node->origin,
+                            nodeIndex, SpecNone, PutStack, origin,
                             OpInfo(m_graph.m_stackAccessData.add(operand, format)),
                             Edge(incoming, uncheckedUseKindFor(format)));
 

Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp

@@ -307,8 +307,8 @@ class SSAConversionPhase : public Phase {
             }
 
             // Insert Phis by asking the calculator what phis there are in this block. Also update
-            // valueForOperand with those Phis. For Phis associated with variables that are not
-            // flushed, we also insert a MovHint.
+            // valueForOperand with those Phis. We also insert a MovHint/PutStack pair to communicate
+            // the phi value to future phases like PutStack sinking.
             size_t phiInsertionPoint = 0;
             for (SSACalculator::Def* phiDef : calculator.phisForBlock(block)) {
                 VariableAccessData* variable = m_variableForSSAIndex[phiDef->variable()->index()];
@@ -319,6 +319,10 @@ class SSAConversionPhase : public Phase {
                 m_insertionSet.insertNode(
                     phiInsertionPoint, SpecNone, MovHint, block->at(0)->origin.withInvalidExit(),
                     OpInfo(variable->operand()), phiDef->value()->defaultEdge());
+                auto format = variable->flushFormat();
+                m_insertionSet.insertNode(
+                    phiInsertionPoint, SpecNone, PutStack, block->at(0)->origin.withInvalidExit(),
+                    OpInfo(m_graph.m_stackAccessData.add(variable->operand(), format)), Edge(phiDef->value(), uncheckedUseKindFor(format)));
             }
 
             if (block->at(0)->origin.exitOK)