Diff
Modified: trunk/JSTests/ChangeLog (283622 => 283623)
--- trunk/JSTests/ChangeLog 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/JSTests/ChangeLog 2021-10-06 15:49:42 UTC (rev 283623)
@@ -1,3 +1,16 @@
+2021-10-06 Saam Barati <sbar...@apple.com>
+
+ Run backwards propagation before we prune the graph after ForceOSRExit nodes in BytecodeParser
+ https://bugs.webkit.org/show_bug.cgi?id=230823
+ <rdar://problem/83565088>
+
+ Reviewed by Robin Morisset.
+
+ * run-backwards-propagation-before-osr-exit-pruning.js: Added.
+ (assert):
+ (main.async v24):
+ (main):
+
2021-10-05 Saam Barati <sbar...@apple.com>
Don't pass DontBuildStrings to next token after parsing an empty parameter list
Added: trunk/JSTests/run-backwards-propagation-before-osr-exit-pruning.js (0 => 283623)
--- trunk/JSTests/run-backwards-propagation-before-osr-exit-pruning.js (rev 0)
+++ trunk/JSTests/run-backwards-propagation-before-osr-exit-pruning.js 2021-10-06 15:49:42 UTC (rev 283623)
@@ -0,0 +1,24 @@
+//@ runDefault("--validateOptions=true", "--useConcurrentJIT=false", "--useConcurrentGC=false", "--thresholdForJITSoon=10", "--thresholdForJITAfterWarmUp=10", "--thresholdForOptimizeAfterWarmUp=100", "--thresholdForOptimizeAfterLongWarmUp=100", "--thresholdForOptimizeSoon=100", "--thresholdForFTLOptimizeAfterWarmUp=1000", "--thresholdForFTLOptimizeSoon=1000", "--validateBCE=true", "--useFTLJIT=true")
+
+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+function main() {
+ let v38;
+ let v40;
+
+ async function v24() {
+ const v33 = false;
+ const v34 = -v33;
+ const v37 = typeof search;
+ const v39 = v38 ? v30 : 1;
+ v40 = v34;
+
+ for (let v41 = 0; v41 != 100000; v41++) { }
+ }
+ [1,1,1].filter(v24);
+ assert(Object.is(v40, -0) === true);
+ assert(Object.is(v40, 0) === false);
+}
+main();
Modified: trunk/Source/_javascript_Core/ChangeLog (283622 => 283623)
--- trunk/Source/_javascript_Core/ChangeLog 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-10-06 15:49:42 UTC (rev 283623)
@@ -1,3 +1,40 @@
+2021-10-06 Saam Barati <sbar...@apple.com>
+
+ Run backwards propagation before we prune the graph after ForceOSRExit nodes in BytecodeParser
+ https://bugs.webkit.org/show_bug.cgi?id=230823
+ <rdar://problem/83565088>
+
+ Reviewed by Robin Morisset.
+
+ We've found yet another bug where pruning code after OSR exits
+ before running backwards propagation leads to us breaking the spec
+ in weird IR situations. In the particular test case here, we end
+ up not thinking we care about negative zero for an ArithNegate,
+ and we exit the program while recovering the value 0 instead of -0.
+
+ Fundamentally, backwards propagation wants to see all bytecode uses.
+ Therefore, it seems like a more sound strategy to run backwards propagation
+ before we end up mucking with the graph. This patch makes it so we run
+ backwards propagation inside bytecode parser before we prune the IR.
+ That way, the phase sees the graph as if it's an IR over the whole bytecode
+ graph.
+
+ * bytecode/Operands.h:
+ (JSC::Operands::operator!= const):
+ * dfg/DFGBackwardsPropagationPhase.cpp:
+ (JSC::DFG::BackwardsPropagationPhase::BackwardsPropagationPhase):
+ (JSC::DFG::BackwardsPropagationPhase::run):
+ (JSC::DFG::BackwardsPropagationPhase::mergeFlags):
+ (JSC::DFG::BackwardsPropagationPhase::propagate):
+ (JSC::DFG::performBackwardsPropagation):
+ * dfg/DFGBackwardsPropagationPhase.h:
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::parse):
+ * dfg/DFGPlan.cpp:
+ (JSC::DFG::Plan::compileInThreadImpl):
+ * dfg/DFGUnificationPhase.cpp:
+ (JSC::DFG::UnificationPhase::run):
+
2021-10-06 Xan López <x...@igalia.com>
[JSC][32bit] Remove a bunch of compiler warnings
Modified: trunk/Source/_javascript_Core/bytecode/Operands.h (283622 => 283623)
--- trunk/Source/_javascript_Core/bytecode/Operands.h 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/bytecode/Operands.h 2021-10-06 15:49:42 UTC (rev 283623)
@@ -400,6 +400,11 @@
return m_values == other.m_values;
}
+
+ bool operator!=(const Operands& other) const
+ {
+ return !(*this == other);
+ }
void dumpInContext(PrintStream& out, DumpContext* context) const;
void dump(PrintStream& out) const;
Modified: trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp (283622 => 283623)
--- trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp 2021-10-06 15:49:42 UTC (rev 283623)
@@ -34,22 +34,48 @@
namespace JSC { namespace DFG {
-class BackwardsPropagationPhase : public Phase {
+// This phase is run at the end of BytecodeParsing, so the graph isn't in a fully formed state.
+// For example, we can't access the predecessor list of any basic blocks yet.
+
+class BackwardsPropagationPhase {
public:
BackwardsPropagationPhase(Graph& graph)
- : Phase(graph, "backwards propagation")
+ : m_graph(graph)
+ , m_flagsAtHead(graph)
{
}
bool run()
{
- m_changed = true;
- while (m_changed) {
- m_changed = false;
+ for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
+ m_flagsAtHead[block] = Operands<NodeFlags>(OperandsLike, m_graph.block(0)->variablesAtHead);
+ m_flagsAtHead[block].fill(0);
+ }
+
+ bool changed;
+ do {
+ changed = false;
+
for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
BasicBlock* block = m_graph.block(blockIndex);
if (!block)
continue;
+
+ {
+ unsigned numSuccessors = block->numSuccessors();
+ if (!numSuccessors) {
+ m_currentFlags = Operands<NodeFlags>(OperandsLike, m_graph.block(0)->variablesAtHead);
+ m_currentFlags.fill(0);
+ } else {
+ m_currentFlags = m_flagsAtHead[block->successor(0)];
+ for (unsigned i = 1; i < numSuccessors; ++i) {
+ BasicBlock* successor = block->successor(i);
+ for (size_t i = 0; i < m_currentFlags.size(); ++i)
+ m_currentFlags[i] |= m_flagsAtHead[successor][i];
+ }
+ }
+ }
+
// Prevent a tower of overflowing additions from creating a value that is out of the
// safe 2^48 range.
@@ -57,8 +83,13 @@
for (unsigned indexInBlock = block->size(); indexInBlock--;)
propagate(block->at(indexInBlock));
+
+ if (m_flagsAtHead[block] != m_currentFlags) {
+ m_flagsAtHead[block] = m_currentFlags;
+ changed = true;
+ }
}
- }
+ } while (changed);
return true;
}
@@ -153,6 +184,11 @@
return isWithinPowerOfTwo<power>(edge.node());
}
+ static bool mergeFlags(NodeFlags& flagsRef, NodeFlags newFlags)
+ {
+ return checkAndSet(flagsRef, flagsRef | newFlags);
+ }
+
bool mergeDefaultFlags(Node* node)
{
bool changed = false;
@@ -184,25 +220,36 @@
switch (node->op()) {
case GetLocal: {
VariableAccessData* variableAccessData = node->variableAccessData();
- flags &= ~NodeBytecodeUsesAsInt; // We don't care about cross-block uses-as-int.
- m_changed |= variableAccessData->mergeFlags(flags);
+ NodeFlags& flagsRef = m_currentFlags.operand(variableAccessData->operand());
+ mergeFlags(flagsRef, flags);
+ variableAccessData->mergeFlags(flagsRef & ~NodeBytecodeUsesAsInt); // We don't care about cross-block uses-as-int for this.
break;
}
case SetLocal: {
VariableAccessData* variableAccessData = node->variableAccessData();
- if (!variableAccessData->isLoadedFrom())
+
+ Operand operand = variableAccessData->operand();
+ NodeFlags flags = m_currentFlags.operand(operand);
+ if (!flags)
break;
- flags = variableAccessData->flags();
+
RELEASE_ASSERT(!(flags & ~NodeBytecodeBackPropMask));
- flags |= NodeBytecodeUsesAsNumber; // Account for the fact that control flow may cause overflows that our modeling can't handle.
- node->child1()->mergeFlags(flags);
+
+ variableAccessData->mergeFlags(flags);
+ // We union with NodeBytecodeUsesAsNumber to account for the fact that control flow may cause overflows that our modeling can't handle.
+ // For example, a loop where we always add a constant value.
+ node->child1()->mergeFlags(flags | NodeBytecodeUsesAsNumber);
+
+ m_currentFlags.operand(operand) = 0;
break;
}
case Flush: {
VariableAccessData* variableAccessData = node->variableAccessData();
- m_changed |= variableAccessData->mergeFlags(NodeBytecodeUsesAsValue);
+ NodeFlags& flagsRef = m_currentFlags.operand(variableAccessData->operand());
+ mergeFlags(flagsRef, NodeBytecodeUsesAsValue);
+ variableAccessData->mergeFlags(flagsRef);
break;
}
@@ -491,13 +538,16 @@
}
}
+ Graph& m_graph;
bool m_allowNestedOverflowingAdditions;
- bool m_changed;
+
+ BlockMap<Operands<NodeFlags>> m_flagsAtHead;
+ Operands<NodeFlags> m_currentFlags;
};
-bool performBackwardsPropagation(Graph& graph)
+void performBackwardsPropagation(Graph& graph)
{
- return runPhase<BackwardsPropagationPhase>(graph);
+ BackwardsPropagationPhase(graph).run();
}
} } // namespace JSC::DFG
Modified: trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.h (283622 => 283623)
--- trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.h 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.h 2021-10-06 15:49:42 UTC (rev 283623)
@@ -35,7 +35,7 @@
// Infer basic information about how nodes are used by doing a block-local
// backwards flow analysis.
-bool performBackwardsPropagation(Graph&);
+void performBackwardsPropagation(Graph&);
} } // namespace JSC::DFG
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (283622 => 283623)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2021-10-06 15:49:42 UTC (rev 283623)
@@ -43,6 +43,7 @@
#include "CommonSlowPaths.h"
#include "DFGAbstractHeap.h"
#include "DFGArrayMode.h"
+#include "DFGBackwardsPropagationPhase.h"
#include "DFGBlockSet.h"
#include "DFGCapabilities.h"
#include "DFGClobberize.h"
@@ -9018,6 +9019,13 @@
parseCodeBlock();
linkBlocks(inlineStackEntry.m_unlinkedBlocks, inlineStackEntry.m_blockLinkingTargets);
+ // We run backwards propagation now because the soundness of that phase
+ // relies on seeing the graph as if it were an IR over bytecode, since
+ // the spec-correctness of that phase relies on seeing all bytecode uses.
+ // Therefore, we run this pass before we do any pruning of the graph
+ // after ForceOSRExit sites.
+ performBackwardsPropagation(m_graph);
+
if (m_hasAnyForceOSRExits) {
BlockSet blocksToIgnore;
for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
@@ -9076,15 +9084,17 @@
++nodeIndex;
+ if (validationEnabled()) {
+ // This verifies that we don't need to change any of the successors's predecessor
+ // list after planting the Unreachable below. At this point in the bytecode
+ // parser, we haven't linked up the predecessor lists yet.
+ for (BasicBlock* successor : block->successors())
+ RELEASE_ASSERT(successor->predecessors.isEmpty());
+ }
+
+ block->resize(nodeIndex);
+
{
- if (validationEnabled()) {
- // This verifies that we don't need to change any of the successors's predecessor
- // list after planting the Unreachable below. At this point in the bytecode
- // parser, we haven't linked up the predecessor lists yet.
- for (BasicBlock* successor : block->successors())
- RELEASE_ASSERT(successor->predecessors.isEmpty());
- }
-
auto insertLivenessPreservingOp = [&] (InlineCallFrame* inlineCallFrame, NodeType op, Operand operand) {
VariableAccessData* variable = mapping.operand(operand);
if (!variable) {
@@ -9108,41 +9118,9 @@
flushForTerminalImpl(origin.semantic, addFlushDirect, addPhantomLocalDirect);
}
- while (true) {
- RELEASE_ASSERT(nodeIndex < block->size());
-
- Node* node = block->at(nodeIndex);
-
- node->origin = origin;
- m_graph.doToChildren(node, [&] (Edge edge) {
- // We only need to keep data flow edges to nodes defined prior to the ForceOSRExit. The reason
- // for this is we rely on backwards propagation being able to see the "full" bytecode. To model
- // this, we preserve uses of a node in a generic way so that backwards propagation can reason
- // about them. Therefore, we can't remove uses of a node which is defined before the ForceOSRExit
- // even when we're at a point in the program after the ForceOSRExit, because that would break backwards
- // propagation's analysis over the uses of a node. However, we don't need this same preservation for
- // nodes defined after ForceOSRExit, as we've already exitted before those defs.
- if (edge->hasResult())
- insertionSet.insertNode(nodeIndex, SpecNone, Phantom, origin, Edge(edge.node(), UntypedUse));
- });
-
- bool isTerminal = node->isTerminal();
-
- node->removeWithoutChecks();
-
- if (isTerminal) {
- insertionSet.insertNode(nodeIndex, SpecNone, Unreachable, origin);
- break;
- }
-
- ++nodeIndex;
- }
-
+ insertionSet.insertNode(nodeIndex, SpecNone, Unreachable, origin);
insertionSet.execute(block);
- auto nodeAndIndex = block->findTerminal();
- DFG_ASSERT(m_graph, nodeAndIndex.node, nodeAndIndex.node->op() == Unreachable);
- block->resize(nodeAndIndex.index + 1);
break;
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGPlan.cpp (283622 => 283623)
--- trunk/Source/_javascript_Core/dfg/DFGPlan.cpp 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/dfg/DFGPlan.cpp 2021-10-06 15:49:42 UTC (rev 283623)
@@ -29,7 +29,6 @@
#if ENABLE(DFG_JIT)
#include "DFGArgumentsEliminationPhase.h"
-#include "DFGBackwardsPropagationPhase.h"
#include "DFGByteCodeParser.h"
#include "DFGCFAPhase.h"
#include "DFGCFGSimplificationPhase.h"
@@ -255,7 +254,6 @@
if (validationEnabled())
validate(dfg);
- RUN_PHASE(performBackwardsPropagation);
RUN_PHASE(performPredictionPropagation);
RUN_PHASE(performFixup);
RUN_PHASE(performInvalidationPointInjection);
Modified: trunk/Source/_javascript_Core/dfg/DFGUnificationPhase.cpp (283622 => 283623)
--- trunk/Source/_javascript_Core/dfg/DFGUnificationPhase.cpp 2021-10-06 15:43:37 UTC (rev 283622)
+++ trunk/Source/_javascript_Core/dfg/DFGUnificationPhase.cpp 2021-10-06 15:49:42 UTC (rev 283623)
@@ -75,6 +75,7 @@
data->find()->mergeShouldNeverUnbox(data->shouldNeverUnbox());
data->find()->mergeIsLoadedFrom(data->isLoadedFrom());
data->find()->mergeIsProfitableToUnbox(data->isProfitableToUnbox());
+ data->find()->mergeFlags(data->flags());
}
m_graph.m_unificationState = GloballyUnified;
