DFG::PutStackSinkingPhase should eliminate GetStacks that have an obviously known...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Mar 2015 17:39:07 +0000 (17:39 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Mar 2015 17:39:07 +0000 (17:39 +0000)
commit1a8fc1957dc539bfc435ddf6e60f4be5fd23a072
treeee03fef270d048ceafde0c06f9e06bf5dd53113e
parentb0ed446a0912c0599b3b784eb067d1f37beb5083
DFG::PutStackSinkingPhase should eliminate GetStacks that have an obviously known source, and emit GetStacks when the stack's value is needed and none is deferred
https://bugs.webkit.org/show_bug.cgi?id=141624

Reviewed by Geoffrey Garen.

Not eliminating GetStacks was an obvious omission from the original PutStackSinkingPhase.
Previously, we would treat GetStacks conservatively and assume that the stack slot
escaped. That's pretty dumb, since a GetStack is a local load of the stack. This change
makes GetStack a no-op from the standpoint of this phase's deferral analysis. At the end
we either keep the GetStack (if there was no concrete deferral) or we replace it with an
identity over the value that would have been stored by the deferred PutStack. Note that
this might be a Phi that the phase creates, so this is strictly stronger than what GCSE
could do.

But this change revealed the fact that this phase never correctly handled side effects in
case that we had done a GetStack, then a side-effect, and then found ourselves wanting the
value on the stack due to (for example) a Phi on a deferred PutStack and that GetStack.
Basically, it's only correct to use the SSA converter's incoming value mapping if we have
a concrete deferral - since anything but a concrete deferral may imply that the value has
been clobbered.

This has no performance change. I believe that the bug was previously benign because we
have so few operations that clobber the stack anymore, and most of those get used in a
very idiomatic way. The GetStack elimination will be very useful for the varargs
simplification that is part of bug 141174.

This includes a test for the case that Speedometer hit, plus tests for the other cases I
thought of once I realized the deeper issue.

* dfg/DFGPutStackSinkingPhase.cpp:
* tests/stress/get-stack-identity-due-to-sinking.js: Added.
(foo):
(bar):
* tests/stress/get-stack-mapping-with-dead-get-stack.js: Added.
(bar):
(foo):
* tests/stress/get-stack-mapping.js: Added.
(bar):
(foo):
* tests/stress/weird-put-stack-varargs.js: Added.
(baz):
(foo):
(fuzz):
(bar):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181563 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp
Source/JavaScriptCore/tests/stress/get-stack-identity-due-to-sinking.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/get-stack-mapping-with-dead-get-stack.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/get-stack-mapping.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/weird-put-stack-varargs.js [new file with mode: 0644]