Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSC] CFA should clear abstract values first before reconstruction #14093

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions JSTests/stress/regress-109263765.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//@requireOptions("--thresholdForFTLOptimizeSoon=0", "--validateAbstractInterpreterState=1")
function foo() {
let a = Object;
let b = Object;
let c = Array.prototype;
for (let i = 0; i < 10; i++) {
for (let j = 0; j < 100; j++) {
let xs = [0, 1];
for (let x in xs) { }
}
}
}

for (let i = 0; i < 100; i++) {
foo();
}
Original file line number Diff line number Diff line change
Expand Up @@ -4614,6 +4614,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
}
}
m_state.setNonCellTypeForTupleNode(node, 0, SpecInt32Only);
clearForNode(node);
break;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ class ArgumentsEliminationPhase : public Phase {
// object's payload. Conservatively this means that the stack region doesn't get stored to.
void eliminateCandidatesThatInterfere()
{
performLivenessAnalysis(m_graph);
performGraphPackingAndLivenessAnalysis(m_graph);
m_graph.initializeNodeOwners();
CombinedLiveness combinedLiveness(m_graph);

Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/dfg/DFGCFAPhase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class CFAPhase : public Phase {
{
ASSERT(m_graph.m_form == ThreadedCPS || m_graph.m_form == SSA);
ASSERT(m_graph.m_unificationState == GloballyUnified);
ASSERT(m_graph.m_refCountState == EverythingIsLive);
ASSERT(m_graph.m_refCountState == EverythingIsLive || Options::validateAbstractInterpreterState() && m_graph.m_refCountState == ExactRefCount);

m_count = 0;

Expand Down
9 changes: 8 additions & 1 deletion Source/JavaScriptCore/dfg/DFGFlowMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,14 @@ class FlowMap {
ALWAYS_INLINE const T& at(unsigned nodeIndex, NodeFlowProjection::Kind kind) const { return const_cast<FlowMap*>(this)->at(nodeIndex, kind); }
ALWAYS_INLINE const T& at(Node* node, NodeFlowProjection::Kind kind) const { return const_cast<FlowMap*>(this)->at(node, kind); }
ALWAYS_INLINE const T& at(NodeFlowProjection projection) const { return const_cast<FlowMap*>(this)->at(projection); }


ALWAYS_INLINE void clear()
{
m_map.clear();
m_shadowMap.clear();
resize();
}

private:
Graph& m_graph;
Vector<T, 0, UnsafeVectorOverflow> m_map;
Expand Down
5 changes: 5 additions & 0 deletions Source/JavaScriptCore/dfg/DFGGraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,11 @@ void Graph::packNodeIndices()
m_nodes.packIndices();
}

void Graph::clearAbstractValues()
{
m_abstractValuesCache->clear();
}

void Graph::dethread()
{
if (m_form == LoadStore || m_form == SSA)
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/dfg/DFGGraph.h
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ class Graph final : public virtual Scannable {
unsigned maxNodeCount() const { return m_nodes.size(); }
Node* nodeAt(unsigned index) const { return m_nodes[index]; }
void packNodeIndices();
void clearAbstractValues();

void dethread();

Expand Down
1 change: 0 additions & 1 deletion Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ class InPlaceAbstractState {

ALWAYS_INLINE void clearForNode(NodeFlowProjection node)
{
ASSERT(!node->isTuple());
AbstractValue& value = m_abstractValues.at(node);
value.clear();
value.m_effectEpoch = m_effectEpoch;
Expand Down
6 changes: 4 additions & 2 deletions Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,12 @@ class LivenessAnalysisPhase : public Phase {

} // anonymous namespace

bool performLivenessAnalysis(Graph& graph)
bool performGraphPackingAndLivenessAnalysis(Graph& graph)
{
graph.packNodeIndices();
hyjorc1 marked this conversation as resolved.
Show resolved Hide resolved

#ifndef NDEBUG
graph.clearAbstractValues();
#endif
return runPhase<LivenessAnalysisPhase>(graph);
}

Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class Graph;

// Computes BasicBlock::ssa->liveAtHead/liveAtTail.

bool performLivenessAnalysis(Graph&);
bool performGraphPackingAndLivenessAnalysis(Graph&);

} } // namespace JSC::DFG

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ class ObjectAllocationSinkingPhase : public Phase {
m_graph.computeRefCounts();
m_graph.initializeNodeOwners();
m_graph.ensureSSADominators();
performLivenessAnalysis(m_graph);
performGraphPackingAndLivenessAnalysis(m_graph);
performOSRAvailabilityAnalysis(m_graph);
m_combinedLiveness = CombinedLiveness(m_graph);

Expand Down
12 changes: 6 additions & 6 deletions Source/JavaScriptCore/dfg/DFGPlan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ Plan::CompilationPath Plan::compileInThreadImpl()

RUN_PHASE(performConstantHoisting);
RUN_PHASE(performGlobalCSE);
RUN_PHASE(performLivenessAnalysis);
RUN_PHASE(performGraphPackingAndLivenessAnalysis);
RUN_PHASE(performCFA);
RUN_PHASE(performConstantFolding);
RUN_PHASE(performCFGSimplification);
Expand All @@ -391,7 +391,7 @@ Plan::CompilationPath Plan::compileInThreadImpl()
if (changed) {
// State-at-tail and state-at-head will be invalid if we did strength reduction since
// it might increase live ranges.
RUN_PHASE(performLivenessAnalysis);
RUN_PHASE(performGraphPackingAndLivenessAnalysis);
RUN_PHASE(performCFA);
RUN_PHASE(performConstantFolding);
RUN_PHASE(performCFGSimplification);
Expand All @@ -402,7 +402,7 @@ Plan::CompilationPath Plan::compileInThreadImpl()
// wrong with running LICM earlier, if we wanted to put other CFG transforms above this point.
// Alternatively, we could run loop pre-header creation after SSA conversion - but if we did that
// then we'd need to do some simple SSA fix-up.
RUN_PHASE(performLivenessAnalysis);
RUN_PHASE(performGraphPackingAndLivenessAnalysis);
RUN_PHASE(performCFA);
RUN_PHASE(performLICM);

Expand All @@ -413,7 +413,7 @@ Plan::CompilationPath Plan::compileInThreadImpl()
// by IntegerRangeOptimization.
//
// Ideally, the dependencies should be explicit. See https://bugs.webkit.org/show_bug.cgi?id=157534.
RUN_PHASE(performLivenessAnalysis);
RUN_PHASE(performGraphPackingAndLivenessAnalysis);
RUN_PHASE(performIntegerRangeOptimization);

RUN_PHASE(performCleanUp);
Expand All @@ -424,14 +424,14 @@ Plan::CompilationPath Plan::compileInThreadImpl()
// about code motion assumes that it's OK to insert GC points in random places.
dfg.m_fixpointState = FixpointConverged;

RUN_PHASE(performLivenessAnalysis);
RUN_PHASE(performGraphPackingAndLivenessAnalysis);
RUN_PHASE(performCFA);
RUN_PHASE(performGlobalStoreBarrierInsertion);
RUN_PHASE(performStoreBarrierClustering);
RUN_PHASE(performCleanUp);
RUN_PHASE(performDCE); // We rely on this to kill dead code that won't be recognized as dead by B3.
RUN_PHASE(performStackLayout);
RUN_PHASE(performLivenessAnalysis);
RUN_PHASE(performGraphPackingAndLivenessAnalysis);
RUN_PHASE(performOSRAvailabilityAnalysis);

if (FTL::canCompile(dfg) == FTL::CannotCompile) {
Expand Down
10 changes: 6 additions & 4 deletions Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "CallFrameShuffler.h"
#include "ClonedArguments.h"
#include "DFGAbstractInterpreterInlines.h"
#include "DFGCFAPhase.h"
#include "DFGCapabilities.h"
#include "DFGClobberize.h"
#include "DFGDoesGC.h"
Expand Down Expand Up @@ -166,8 +167,9 @@ class LowerDFGToB3 {
, m_state(state.graph)
, m_interpreter(state.graph, m_state)
{
if (Options::validateAbstractInterpreterState()) {
performLivenessAnalysis(m_graph);
if (UNLIKELY(Options::validateAbstractInterpreterState())) {
performGraphPackingAndLivenessAnalysis(m_graph);
performCFA(m_graph);

// We only use node liveness here, not combined liveness, as we only track
// AI state for live nodes.
Expand Down Expand Up @@ -615,7 +617,7 @@ class LowerDFGToB3 {
continue;
if (node->op() == CheckInBoundsInt52)
continue;
if (node->op() == EnumeratorNextUpdateIndexAndMode) {
if (node->isTuple()) {
ASSERT(m_interpreter.hasClearedAbstractState(node));
continue;
}
Expand Down Expand Up @@ -726,7 +728,7 @@ class LowerDFGToB3 {
m_interpreter.startExecuting();
m_interpreter.executeKnownEdgeTypes(m_node);

if (Options::validateAbstractInterpreterState())
if (UNLIKELY(Options::validateAbstractInterpreterState()))
validateAIState(m_node);

if constexpr (validateDFGDoesGC) {
Expand Down