WebKit Bugzilla
Attachment 371256 Details for
Bug 198520
: Argument elimination should check transitive dependents for interference
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-198520-20190604111028.patch (text/plain), 13.56 KB, created by
Tadeu Zagallo
on 2019-06-04 02:10:30 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Tadeu Zagallo
Created:
2019-06-04 02:10:30 PDT
Size:
13.56 KB
patch
obsolete
>Subversion Revision: 245778 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index df5adc4cc725fa461b2ac8974f772558f11a5455..0ffd0a0beb7febaa2d0d044f3e983a4e846295ee 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,31 @@ >+2019-06-04 Tadeu Zagallo <tzagallo@apple.com> >+ >+ Argument elimination should check transitive dependents for interference >+ https://bugs.webkit.org/show_bug.cgi?id=198520 >+ <rdar://problem/50863343> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Consider the following program: >+ >+ a: CreateRest >+ --> >+ b: CreateRest >+ <-- >+ c: Spread(@a) >+ d: Spread(@b) >+ e: NewArrayWithSpread(@a, @b) >+ f: KillStack(locX) >+ g: LoadVarargs(@e) >+ >+ Suppose @b reads locX, then we cannot transform @e to PhantomNewArraySpread, since that would >+ move the stack access from @b into @g, and that stack location is no longer valid at that point. >+ >+ We fix that by computing a set of all inline call frames that any argument elimination candidate >+ depends on and checking each of them for interference in `eliminateCandidatesThatInterfere`. >+ >+ * dfg/DFGArgumentsEliminationPhase.cpp: >+ > 2019-05-25 Tadeu Zagallo <tzagallo@apple.com> > > JITOperations getByVal should mark negative array indices as out-of-bounds >diff --git a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp >index 2b7a7c651b48fedb140449f9d46db7634b7f09f4..0ad92aae0a6ed3dd09c06c7c75205ea93b71b1df 100644 >--- a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp >+++ b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp >@@ -502,6 +502,33 @@ private: > } > } > >+ using InlineCallFrames = HashSet<InlineCallFrame*, WTF::DefaultHash<InlineCallFrame*>::Hash, WTF::NullableHashTraits<InlineCallFrame*>>; >+ using InlineCallFramesForCanditates = HashMap<Node*, InlineCallFrames>; >+ InlineCallFramesForCanditates inlineCallFramesForCandidate; >+ auto forEachDependentNode = recursableLambda([&](auto self, Node* node, const auto& functor) -> void { >+ functor(node); >+ >+ if (node->op() == Spread) { >+ self(node->child1().node(), functor); >+ return; >+ } >+ >+ if (node->op() == NewArrayWithSpread) { >+ BitVector* bitVector = node->bitVector(); >+ for (unsigned i = node->numChildren(); i--; ) { >+ if (bitVector->get(i)) >+ self(m_graph.varArgChild(node, i).node(), functor); >+ } >+ return; >+ } >+ }); >+ for (Node* candidate : m_candidates) { >+ auto& set = inlineCallFramesForCandidate.add(candidate, InlineCallFrames()).iterator->value; >+ forEachDependentNode(candidate, [&](Node* dependent) { >+ set.add(dependent->origin.semantic.inlineCallFrame()); >+ }); >+ } >+ > for (BasicBlock* block : m_graph.blocksInNaturalOrder()) { > // Stop if we've already removed all candidates. > if (m_candidates.isEmpty()) >@@ -524,83 +551,88 @@ private: > if (!m_candidates.contains(candidate)) > return; > >- // Check if this block has any clobbers that affect this candidate. This is a fairly >- // fast check. >- bool isClobberedByBlock = false; >- Operands<bool>& clobberedByThisBlock = clobberedByBlock[block]; >- >- if (InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame()) { >- if (inlineCallFrame->isVarargs()) { >- isClobberedByBlock |= clobberedByThisBlock.operand( >- inlineCallFrame->stackOffset + CallFrameSlot::argumentCount); >- } >- >- if (!isClobberedByBlock || inlineCallFrame->isClosureCall) { >- isClobberedByBlock |= clobberedByThisBlock.operand( >- inlineCallFrame->stackOffset + CallFrameSlot::callee); >- } >+ for (InlineCallFrame* inlineCallFrame : inlineCallFramesForCandidate.get(candidate)) { >+ // Check if this block has any clobbers that affect this candidate. This is a fairly >+ // fast check. >+ bool isClobberedByBlock = false; >+ Operands<bool>& clobberedByThisBlock = clobberedByBlock[block]; > >- if (!isClobberedByBlock) { >- for (unsigned i = 0; i < inlineCallFrame->argumentCountIncludingThis - 1; ++i) { >- VirtualRegister reg = >- VirtualRegister(inlineCallFrame->stackOffset) + >- CallFrame::argumentOffset(i); >- if (clobberedByThisBlock.operand(reg)) { >+ if (inlineCallFrame) { >+ if (inlineCallFrame->isVarargs()) { >+ isClobberedByBlock |= clobberedByThisBlock.operand( >+ inlineCallFrame->stackOffset + CallFrameSlot::argumentCount); >+ } >+ >+ if (!isClobberedByBlock || inlineCallFrame->isClosureCall) { >+ isClobberedByBlock |= clobberedByThisBlock.operand( >+ inlineCallFrame->stackOffset + CallFrameSlot::callee); >+ } >+ >+ if (!isClobberedByBlock) { >+ for (unsigned i = 0; i < inlineCallFrame->argumentCountIncludingThis - 1; ++i) { >+ VirtualRegister reg = >+ VirtualRegister(inlineCallFrame->stackOffset) + >+ CallFrame::argumentOffset(i); >+ if (clobberedByThisBlock.operand(reg)) { >+ isClobberedByBlock = true; >+ break; >+ } >+ } >+ } >+ } else { >+ // We don't include the ArgumentCount or Callee in this case because we can be >+ // damn sure that this won't be clobbered. >+ for (unsigned i = 1; i < static_cast<unsigned>(codeBlock()->numParameters()); ++i) { >+ if (clobberedByThisBlock.argument(i)) { > isClobberedByBlock = true; > break; > } > } > } >- } else { >- // We don't include the ArgumentCount or Callee in this case because we can be >- // damn sure that this won't be clobbered. >- for (unsigned i = 1; i < static_cast<unsigned>(codeBlock()->numParameters()); ++i) { >- if (clobberedByThisBlock.argument(i)) { >- isClobberedByBlock = true; >- break; >- } >- } >- } >- >- if (!isClobberedByBlock) >- return; >- >- // Check if we can immediately eliminate this candidate. If the block has a clobber >- // for this arguments allocation, and we'd have to examine every node in the block, >- // then we can just eliminate the candidate. >- if (nodeIndex == block->size() && candidate->owner != block) { >- if (DFGArgumentsEliminationPhaseInternal::verbose) >- dataLog("eliminating candidate: ", candidate, " because it is clobbered by: ", block->at(nodeIndex), "\n"); >- transitivelyRemoveCandidate(candidate); >- return; >- } >- >- // This loop considers all nodes up to the nodeIndex, excluding the nodeIndex. >- while (nodeIndex--) { >- Node* node = block->at(nodeIndex); >- if (node == candidate) >- break; > >- bool found = false; >- clobberize( >- m_graph, node, NoOpClobberize(), >- [&] (AbstractHeap heap) { >- if (heap.kind() == Stack && !heap.payload().isTop()) { >- if (argumentsInvolveStackSlot(candidate, VirtualRegister(heap.payload().value32()))) >- found = true; >- return; >- } >- if (heap.overlaps(Stack)) >- found = true; >- }, >- NoOpClobberize()); >+ if (!isClobberedByBlock) >+ continue; > >- if (found) { >+ // Check if we can immediately eliminate this candidate. If the block has a clobber >+ // for this arguments allocation, and we'd have to examine every node in the block, >+ // then we can just eliminate the candidate. >+ if (nodeIndex == block->size() && candidate->owner != block) { > if (DFGArgumentsEliminationPhaseInternal::verbose) >- dataLog("eliminating candidate: ", candidate, " because it is clobbered by ", block->at(nodeIndex), "\n"); >+ dataLog("eliminating candidate: ", candidate, " because it is clobbered by: ", block->at(nodeIndex), "\n"); > transitivelyRemoveCandidate(candidate); > return; > } >+ >+ // This loop considers all nodes up to the nodeIndex, excluding the nodeIndex. >+ while (nodeIndex--) { >+ Node* node = block->at(nodeIndex); >+ if (node == candidate && inlineCallFrame == candidate->origin.semantic.inlineCallFrame()) >+ break; >+ >+ bool found = false; >+ clobberize( >+ m_graph, node, NoOpClobberize(), >+ [&] (AbstractHeap heap) { >+ if (heap.kind() == Stack && !heap.payload().isTop()) { >+ if (argumentsInvolveStackSlot(inlineCallFrame, VirtualRegister(heap.payload().value32()))) >+ found = true; >+ return; >+ } >+ if (heap.overlaps(Stack)) >+ found = true; >+ }, >+ NoOpClobberize()); >+ >+ if (found) { >+ if (DFGArgumentsEliminationPhaseInternal::verbose) >+ dataLog("eliminating candidate: ", candidate, " because it is clobbered by ", block->at(nodeIndex), "\n"); >+ transitivelyRemoveCandidate(candidate); >+ return; >+ } >+ >+ if (node == candidate) >+ break; >+ } > } > }); > } >diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog >index 96d083c93a85dfc81c589526c3a439dbd8bd881a..94de19b92f8e0721d89e6a15cadcde199adc8c1e 100644 >--- a/JSTests/ChangeLog >+++ b/JSTests/ChangeLog >@@ -1,3 +1,15 @@ >+2019-06-04 Tadeu Zagallo <tzagallo@apple.com> >+ >+ Argument elimination should check transitive dependents for interference >+ https://bugs.webkit.org/show_bug.cgi?id=198520 >+ <rdar://problem/50863343> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * stress/argument-elimination-inline-rest-past-kill.js: Added. >+ (f2): >+ (f3): >+ > 2019-05-25 Tadeu Zagallo <tzagallo@apple.com> > > JITOperations getByVal should mark negative array indices as out-of-bounds >diff --git a/JSTests/stress/argument-elimination-inline-rest-past-kill.js b/JSTests/stress/argument-elimination-inline-rest-past-kill.js >new file mode 100644 >index 0000000000000000000000000000000000000000..69a521fb3e66f16b6872541e177fe36484e9f047 >--- /dev/null >+++ b/JSTests/stress/argument-elimination-inline-rest-past-kill.js >@@ -0,0 +1,16 @@ >+//@ requireOptions("--forceEagerCompilation=1") >+function f2(...a1) { >+ return a1; >+} >+ >+function f3(...a2) { >+ let v1 = f2([]); >+ return f2(...a2, ...v1); >+} >+noInline(f3); >+ >+for (let i = 0; i < 1e5; i++) { >+ var v3 = f3(); >+ if (!Array.isArray(v3[v3.length - 1])) >+ throw new Error('Should be an array'); >+}
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 198520
:
371256
|
371309