Skip to content

Commit 44cf6e0

Browse files
author
Filip Pizlo
committed
DFG should only have two mechanisms for describing effectfulness of nodes; previously there were three
https://bugs.webkit.org/show_bug.cgi?id=141369 Reviewed by Michael Saboff. We previously used the NodeMightClobber and NodeClobbersWorld NodeFlags to describe effectfulness. Starting over a year ago, we introduced a more powerful mechanism - the DFG::clobberize() function. Now we only have one remaining client of the old NodeFlags, and everyone else uses DFG::clobberize(). We should get rid of those NodeFlags and finally switch everyone over to DFG::clobberize(). Unfortunately there is still another place where effectfulness of nodes is described: the AbstractInterpreter. This is because the AbstractInterpreter has special tuning both for compile time performance and there are places where the AI is more precise than clobberize() because of its flow-sensitivity. This means that after this change there will be only two places, rather than three, where the effectfulness of a node has to be described: - DFG::clobberize() - DFG::AbstractInterpreter * dfg/DFGClobberize.cpp: (JSC::DFG::clobbersWorld): * dfg/DFGClobberize.h: * dfg/DFGDoesGC.cpp: (JSC::DFG::doesGC): * dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::fixupNode): (JSC::DFG::FixupPhase::attemptToMakeGetTypedArrayByteLength): (JSC::DFG::FixupPhase::convertToGetArrayLength): (JSC::DFG::FixupPhase::attemptToMakeGetTypedArrayByteOffset): * dfg/DFGGraph.h: (JSC::DFG::Graph::isPredictedNumerical): Deleted. (JSC::DFG::Graph::byValIsPure): Deleted. (JSC::DFG::Graph::clobbersWorld): Deleted. * dfg/DFGNode.h: (JSC::DFG::Node::convertToConstant): (JSC::DFG::Node::convertToGetLocalUnlinked): (JSC::DFG::Node::convertToGetByOffset): (JSC::DFG::Node::convertToMultiGetByOffset): (JSC::DFG::Node::convertToPutByOffset): (JSC::DFG::Node::convertToMultiPutByOffset): * dfg/DFGNodeFlags.cpp: (JSC::DFG::dumpNodeFlags): * dfg/DFGNodeFlags.h: * dfg/DFGNodeType.h: Canonical link: https://commits.webkit.org/159444@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@179840 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent dde2742 commit 44cf6e0

10 files changed

Lines changed: 119 additions & 129 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,53 @@
1+
2015-02-08 Filip Pizlo <fpizlo@apple.com>
2+
3+
DFG should only have two mechanisms for describing effectfulness of nodes; previously there were three
4+
https://bugs.webkit.org/show_bug.cgi?id=141369
5+
6+
Reviewed by Michael Saboff.
7+
8+
We previously used the NodeMightClobber and NodeClobbersWorld NodeFlags to describe
9+
effectfulness. Starting over a year ago, we introduced a more powerful mechanism - the
10+
DFG::clobberize() function. Now we only have one remaining client of the old NodeFlags,
11+
and everyone else uses DFG::clobberize(). We should get rid of those NodeFlags and
12+
finally switch everyone over to DFG::clobberize().
13+
14+
Unfortunately there is still another place where effectfulness of nodes is described: the
15+
AbstractInterpreter. This is because the AbstractInterpreter has special tuning both for
16+
compile time performance and there are places where the AI is more precise than
17+
clobberize() because of its flow-sensitivity.
18+
19+
This means that after this change there will be only two places, rather than three, where
20+
the effectfulness of a node has to be described:
21+
22+
- DFG::clobberize()
23+
- DFG::AbstractInterpreter
24+
25+
* dfg/DFGClobberize.cpp:
26+
(JSC::DFG::clobbersWorld):
27+
* dfg/DFGClobberize.h:
28+
* dfg/DFGDoesGC.cpp:
29+
(JSC::DFG::doesGC):
30+
* dfg/DFGFixupPhase.cpp:
31+
(JSC::DFG::FixupPhase::fixupNode):
32+
(JSC::DFG::FixupPhase::attemptToMakeGetTypedArrayByteLength):
33+
(JSC::DFG::FixupPhase::convertToGetArrayLength):
34+
(JSC::DFG::FixupPhase::attemptToMakeGetTypedArrayByteOffset):
35+
* dfg/DFGGraph.h:
36+
(JSC::DFG::Graph::isPredictedNumerical): Deleted.
37+
(JSC::DFG::Graph::byValIsPure): Deleted.
38+
(JSC::DFG::Graph::clobbersWorld): Deleted.
39+
* dfg/DFGNode.h:
40+
(JSC::DFG::Node::convertToConstant):
41+
(JSC::DFG::Node::convertToGetLocalUnlinked):
42+
(JSC::DFG::Node::convertToGetByOffset):
43+
(JSC::DFG::Node::convertToMultiGetByOffset):
44+
(JSC::DFG::Node::convertToPutByOffset):
45+
(JSC::DFG::Node::convertToMultiPutByOffset):
46+
* dfg/DFGNodeFlags.cpp:
47+
(JSC::DFG::dumpNodeFlags):
48+
* dfg/DFGNodeFlags.h:
49+
* dfg/DFGNodeType.h:
50+
151
2015-02-09 Csaba Osztrogonác <ossy@webkit.org>
252

353
Fix the !ENABLE(DFG_JIT) build

‎Source/JavaScriptCore/dfg/DFGClobberize.cpp‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
2+
* Copyright (C) 2013-2015 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -56,6 +56,19 @@ bool writesOverlap(Graph& graph, Node* node, AbstractHeap heap)
5656
return addWrite.result();
5757
}
5858

59+
bool clobbersWorld(Graph& graph, Node* node)
60+
{
61+
bool result = false;
62+
clobberize(
63+
graph, node, NoOpClobberize(),
64+
[&] (AbstractHeap heap) {
65+
if (heap == AbstractHeap(World))
66+
result = true;
67+
},
68+
NoOpClobberize());
69+
return result;
70+
}
71+
5972
} } // namespace JSC::DFG
6073

6174
#endif // ENABLE(DFG_JIT)

‎Source/JavaScriptCore/dfg/DFGClobberize.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
2+
* Copyright (C) 2013-2015 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -934,6 +934,8 @@ class AbstractHeapOverlaps {
934934
bool accessesOverlap(Graph&, Node*, AbstractHeap);
935935
bool writesOverlap(Graph&, Node*, AbstractHeap);
936936

937+
bool clobbersWorld(Graph&, Node*);
938+
937939
// We would have used bind() for these, but because of the overlaoding that we are doing,
938940
// it's quite a bit of clearer to just write this out the traditional way.
939941

‎Source/JavaScriptCore/dfg/DFGDoesGC.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2014 Apple Inc. All rights reserved.
2+
* Copyright (C) 2014, 2015 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -28,6 +28,7 @@
2828

2929
#if ENABLE(DFG_JIT)
3030

31+
#include "DFGClobberize.h"
3132
#include "DFGGraph.h"
3233
#include "DFGNode.h"
3334
#include "Operations.h"
@@ -36,7 +37,7 @@ namespace JSC { namespace DFG {
3637

3738
bool doesGC(Graph& graph, Node* node)
3839
{
39-
if (graph.clobbersWorld(node))
40+
if (clobbersWorld(graph, node))
4041
return true;
4142

4243
// Now consider nodes that don't clobber the world but that still may GC. This includes all

‎Source/JavaScriptCore/dfg/DFGFixupPhase.cpp‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2012, 2013, 2014 Apple Inc. All rights reserved.
2+
* Copyright (C) 2012-2015 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -161,14 +161,14 @@ class FixupPhase : public Phase {
161161
case ValueAdd: {
162162
if (attemptToMakeIntegerAdd(node)) {
163163
node->setOp(ArithAdd);
164-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
164+
node->clearFlags(NodeMustGenerate);
165165
break;
166166
}
167167
if (Node::shouldSpeculateNumberOrBooleanExpectingDefined(node->child1().node(), node->child2().node())) {
168168
fixDoubleOrBooleanEdge(node->child1());
169169
fixDoubleOrBooleanEdge(node->child2());
170170
node->setOp(ArithAdd);
171-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
171+
node->clearFlags(NodeMustGenerate);
172172
node->setResult(NodeResultDouble);
173173
break;
174174
}
@@ -382,58 +382,58 @@ class FixupPhase : public Phase {
382382
&& Node::shouldSpeculateBoolean(node->child1().node(), node->child2().node())) {
383383
fixEdge<BooleanUse>(node->child1());
384384
fixEdge<BooleanUse>(node->child2());
385-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
385+
node->clearFlags(NodeMustGenerate);
386386
break;
387387
}
388388
if (Node::shouldSpeculateInt32OrBoolean(node->child1().node(), node->child2().node())) {
389389
fixIntOrBooleanEdge(node->child1());
390390
fixIntOrBooleanEdge(node->child2());
391-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
391+
node->clearFlags(NodeMustGenerate);
392392
break;
393393
}
394394
if (enableInt52()
395395
&& Node::shouldSpeculateMachineInt(node->child1().node(), node->child2().node())) {
396396
fixEdge<Int52RepUse>(node->child1());
397397
fixEdge<Int52RepUse>(node->child2());
398-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
398+
node->clearFlags(NodeMustGenerate);
399399
break;
400400
}
401401
if (Node::shouldSpeculateNumberOrBoolean(node->child1().node(), node->child2().node())) {
402402
fixDoubleOrBooleanEdge(node->child1());
403403
fixDoubleOrBooleanEdge(node->child2());
404-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
404+
node->clearFlags(NodeMustGenerate);
405405
break;
406406
}
407407
if (node->op() != CompareEq)
408408
break;
409409
if (node->child1()->shouldSpeculateStringIdent() && node->child2()->shouldSpeculateStringIdent()) {
410410
fixEdge<StringIdentUse>(node->child1());
411411
fixEdge<StringIdentUse>(node->child2());
412-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
412+
node->clearFlags(NodeMustGenerate);
413413
break;
414414
}
415415
if (node->child1()->shouldSpeculateString() && node->child2()->shouldSpeculateString() && GPRInfo::numberOfRegisters >= 7) {
416416
fixEdge<StringUse>(node->child1());
417417
fixEdge<StringUse>(node->child2());
418-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
418+
node->clearFlags(NodeMustGenerate);
419419
break;
420420
}
421421
if (node->child1()->shouldSpeculateObject() && node->child2()->shouldSpeculateObject()) {
422422
fixEdge<ObjectUse>(node->child1());
423423
fixEdge<ObjectUse>(node->child2());
424-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
424+
node->clearFlags(NodeMustGenerate);
425425
break;
426426
}
427427
if (node->child1()->shouldSpeculateObject() && node->child2()->shouldSpeculateObjectOrOther()) {
428428
fixEdge<ObjectUse>(node->child1());
429429
fixEdge<ObjectOrOtherUse>(node->child2());
430-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
430+
node->clearFlags(NodeMustGenerate);
431431
break;
432432
}
433433
if (node->child1()->shouldSpeculateObjectOrOther() && node->child2()->shouldSpeculateObject()) {
434434
fixEdge<ObjectOrOtherUse>(node->child1());
435435
fixEdge<ObjectUse>(node->child2());
436-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
436+
node->clearFlags(NodeMustGenerate);
437437
break;
438438
}
439439
break;
@@ -829,7 +829,7 @@ class FixupPhase : public Phase {
829829
case NewTypedArray: {
830830
if (node->child1()->shouldSpeculateInt32()) {
831831
fixEdge<Int32Use>(node->child1());
832-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
832+
node->clearFlags(NodeMustGenerate);
833833
break;
834834
}
835835
break;
@@ -1947,7 +1947,7 @@ class FixupPhase : public Phase {
19471947
// We can use a BitLShift here because typed arrays will never have a byteLength
19481948
// that overflows int32.
19491949
node->setOp(BitLShift);
1950-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
1950+
node->clearFlags(NodeMustGenerate);
19511951
observeUseKindOnNode(length, Int32Use);
19521952
observeUseKindOnNode(shiftAmount, Int32Use);
19531953
node->child1() = Edge(length, Int32Use);
@@ -1958,7 +1958,7 @@ class FixupPhase : public Phase {
19581958
void convertToGetArrayLength(Node* node, ArrayMode arrayMode)
19591959
{
19601960
node->setOp(GetArrayLength);
1961-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
1961+
node->clearFlags(NodeMustGenerate);
19621962
fixEdge<KnownCellUse>(node->child1());
19631963
node->setArrayMode(arrayMode);
19641964

@@ -1991,7 +1991,7 @@ class FixupPhase : public Phase {
19911991
0, neverNeedsStorage);
19921992

19931993
node->setOp(GetTypedArrayByteOffset);
1994-
node->clearFlags(NodeMustGenerate | NodeClobbersWorld);
1994+
node->clearFlags(NodeMustGenerate);
19951995
fixEdge<KnownCellUse>(node->child1());
19961996
return true;
19971997
}

‎Source/JavaScriptCore/dfg/DFGGraph.h‎

Lines changed: 0 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -545,72 +545,6 @@ class Graph : public virtual Scannable {
545545

546546
void killUnreachableBlocks();
547547

548-
bool isPredictedNumerical(Node* node)
549-
{
550-
return isNumerical(node->child1().useKind()) && isNumerical(node->child2().useKind());
551-
}
552-
553-
// Note that a 'true' return does not actually mean that the ByVal access clobbers nothing.
554-
// It really means that it will not clobber the entire world. It's still up to you to
555-
// carefully consider things like:
556-
// - PutByVal definitely changes the array it stores to, and may even change its length.
557-
// - PutByOffset definitely changes the object it stores to.
558-
// - and so on.
559-
bool byValIsPure(Node* node)
560-
{
561-
switch (node->arrayMode().type()) {
562-
case Array::Generic:
563-
return false;
564-
case Array::Int32:
565-
case Array::Double:
566-
case Array::Contiguous:
567-
case Array::ArrayStorage:
568-
return !node->arrayMode().isOutOfBounds();
569-
case Array::SlowPutArrayStorage:
570-
return !node->arrayMode().mayStoreToHole();
571-
case Array::String:
572-
return node->op() == GetByVal && node->arrayMode().isInBounds();
573-
#if USE(JSVALUE32_64)
574-
case Array::Arguments:
575-
if (node->op() == GetByVal)
576-
return true;
577-
return false;
578-
#endif // USE(JSVALUE32_64)
579-
default:
580-
return true;
581-
}
582-
}
583-
584-
bool clobbersWorld(Node* node)
585-
{
586-
if (node->flags() & NodeClobbersWorld)
587-
return true;
588-
if (!(node->flags() & NodeMightClobber))
589-
return false;
590-
switch (node->op()) {
591-
case GetByVal:
592-
case PutByValDirect:
593-
case PutByVal:
594-
case PutByValAlias:
595-
return !byValIsPure(node);
596-
case ToString:
597-
switch (node->child1().useKind()) {
598-
case StringObjectUse:
599-
case StringOrStringObjectUse:
600-
return false;
601-
case CellUse:
602-
case UntypedUse:
603-
return true;
604-
default:
605-
RELEASE_ASSERT_NOT_REACHED();
606-
return true;
607-
}
608-
default:
609-
RELEASE_ASSERT_NOT_REACHED();
610-
return true; // If by some oddity we hit this case in release build it's safer to have CSE assume the worst.
611-
}
612-
}
613-
614548
void determineReachability();
615549
void resetReachability();
616550

‎Source/JavaScriptCore/dfg/DFGNode.h‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ struct Node {
448448
m_op = Int52Constant;
449449
else
450450
m_op = JSConstant;
451-
m_flags &= ~(NodeMustGenerate | NodeMightClobber | NodeClobbersWorld);
451+
m_flags &= ~NodeMustGenerate;
452452
m_opInfo = bitwise_cast<uintptr_t>(value);
453453
children.reset();
454454
}
@@ -464,7 +464,7 @@ struct Node {
464464
void convertToGetLocalUnlinked(VirtualRegister local)
465465
{
466466
m_op = GetLocalUnlinked;
467-
m_flags &= ~(NodeMustGenerate | NodeMightClobber | NodeClobbersWorld);
467+
m_flags &= ~NodeMustGenerate;
468468
m_opInfo = local.offset();
469469
m_opInfo2 = VirtualRegister().offset();
470470
children.reset();
@@ -478,7 +478,7 @@ struct Node {
478478
children.child2().setUseKind(KnownCellUse);
479479
children.setChild1(storage);
480480
m_op = GetByOffset;
481-
m_flags &= ~(NodeClobbersWorld | NodeMustGenerate);
481+
m_flags &= ~NodeMustGenerate;
482482
}
483483

484484
void convertToMultiGetByOffset(MultiGetByOffsetData* data)
@@ -487,7 +487,6 @@ struct Node {
487487
m_opInfo = bitwise_cast<intptr_t>(data);
488488
child1().setUseKind(CellUse);
489489
m_op = MultiGetByOffset;
490-
m_flags &= ~NodeClobbersWorld;
491490
ASSERT(m_flags & NodeMustGenerate);
492491
}
493492

@@ -499,15 +498,13 @@ struct Node {
499498
children.setChild2(children.child1());
500499
children.setChild1(storage);
501500
m_op = PutByOffset;
502-
m_flags &= ~NodeClobbersWorld;
503501
}
504502

505503
void convertToMultiPutByOffset(MultiPutByOffsetData* data)
506504
{
507505
ASSERT(m_op == PutById || m_op == PutByIdDirect || m_op == PutByIdFlush);
508506
m_opInfo = bitwise_cast<intptr_t>(data);
509507
m_op = MultiPutByOffset;
510-
m_flags &= ~NodeClobbersWorld;
511508
}
512509

513510
void convertToPutByOffsetHint()

‎Source/JavaScriptCore/dfg/DFGNodeFlags.cpp‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,6 @@ void dumpNodeFlags(PrintStream& actualOut, NodeFlags flags)
7474
if (flags & NodeHasVarArgs)
7575
out.print(comma, "VarArgs");
7676

77-
if (flags & NodeClobbersWorld)
78-
out.print(comma, "Clobbers");
79-
80-
if (flags & NodeMightClobber)
81-
out.print(comma, "MightClobber");
82-
8377
if (flags & NodeResultMask) {
8478
if (!(flags & NodeBytecodeUsesAsNumber) && !(flags & NodeBytecodeNeedsNegZero))
8579
out.print(comma, "PureInt");

0 commit comments

Comments
 (0)