Add %GetCallerJSFunction intrinsic

Add %GetCallerJSFunction intrinsic

Only optimized for TF

R=mstarzinger@chromium.org

Committed: https://crrev.com/74f9d8c901ba68c2d664b6e1e3ba80cf0efe8e02
Cr-Commit-Position: refs/heads/master@{#28812}

[danno, 2015-05-20 13:58:05]

PTAL

[Sven Panne, 2015-05-20 14:05:47]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2015-05-20 14:05:48]

DBC...

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
File test/mjsunit/get-caller-js-function.js (right):

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
test/mjsunit/get-caller-js-function.js:8: return %GetCallerJSFunction();
This will *always* call the runtime function, do you mean %_GetCallerJSFunction?

[Michael Starzinger, 2015-05-20 14:25:18]

One real issue about arguments adaptor and construct frames. Otherwise just
nits.

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:470:
NodeProperties::ReplaceWithValue(node, outer_frame, effect, control);
nit: Please use AdvancedReducer::ReplaceWithValue here, I am currently switching
the reducers over to this method, the old NodeProperties::ReplaceWithValue is
deprecated.

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:486: access =
AccessBuilder::ForFrameMarker();
Does this work with arguments adapter frames and construct frames? Can we have
tests for those?

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1045: case IrOpcode::kLoadFramePointer:
nit: Lets just have both labels (i.e. kLoadStackPointer and kLoadFramePointer)
point to one single return statement.

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
File test/mjsunit/get-caller-js-function.js (right):

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
test/mjsunit/get-caller-js-function.js:18: c();
nit: Can we wrap all three calls to "c" into an assertEquals checking the return
value?

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
test/mjsunit/get-caller-js-function.js:28: d();
nit: Likewise.

[danno, 2015-05-22 11:21:35]

Please take another look.

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:470:
NodeProperties::ReplaceWithValue(node, outer_frame, effect, control);
On 2015/05/20 14:25:18, Michael Starzinger wrote:
> nit: Please use AdvancedReducer::ReplaceWithValue here, I am currently
switching
> the reducers over to this method, the old NodeProperties::ReplaceWithValue is
> deprecated.

Done.

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:486: access =
AccessBuilder::ForFrameMarker();
On 2015/05/20 14:25:18, Michael Starzinger wrote:
> Does this work with arguments adapter frames and construct frames? Can we have
> tests for those?

Now it does :-)

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/1146963002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1045: case IrOpcode::kLoadFramePointer:
On 2015/05/20 14:25:18, Michael Starzinger wrote:
> nit: Lets just have both labels (i.e. kLoadStackPointer and kLoadFramePointer)
> point to one single return statement.

Done.

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
File test/mjsunit/get-caller-js-function.js (right):

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
test/mjsunit/get-caller-js-function.js:8: return %GetCallerJSFunction();
On 2015/05/20 14:05:48, Sven Panne wrote:
> This will *always* call the runtime function, do you mean
%_GetCallerJSFunction?

Good catch. Will fix.

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
test/mjsunit/get-caller-js-function.js:18: c();
On 2015/05/20 14:25:18, Michael Starzinger wrote:
> nit: Can we wrap all three calls to "c" into an assertEquals checking the
return
> value?

Done.

https://codereview.chromium.org/1146963002/diff/20001/test/mjsunit/get-caller...
test/mjsunit/get-caller-js-function.js:28: d();
On 2015/05/20 14:25:18, Michael Starzinger wrote:
> nit: Likewise.

Done.

[Michael Starzinger, 2015-05-22 12:41:13]

Looking good.

https://codereview.chromium.org/1146963002/diff/120001/test/mjsunit/get-calle...
File test/mjsunit/get-caller-js-function.js (right):

https://codereview.chromium.org/1146963002/diff/120001/test/mjsunit/get-calle...
test/mjsunit/get-caller-js-function.js:47: return a(1,2,3)
As discussed offline: Can we have another test case the calls "a" as a
constructor using "return new a()"?

[danno, 2015-06-03 14:17:38]

Please take another look

[Michael Starzinger, 2015-06-05 11:21:57]

LGTM with comments.

https://codereview.chromium.org/1146963002/diff/160001/src/compiler/js-intrin...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1146963002/diff/160001/src/compiler/js-intrin...
src/compiler/js-intrinsic-lowering.cc:464: if (FLAG_turbo_deoptimization) {
This flag no longer exists, we can just perform this step unconditionally now.

https://codereview.chromium.org/1146963002/diff/160001/src/compiler/js-intrin...
src/compiler/js-intrinsic-lowering.cc:479: // instruction selection, so that
there is no phase-ordering dependency.
nit: The TODO now no longer applies, correct?

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:424: if (it.frame()->type() != StackFrame::STUB)
{
nit: Let's use RUNTIME_ASSERT(it.frame()->type() == StackFrame::STUB)

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:428: if (it.frame()->type() !=
StackFrame::JAVA_SCRIPT) {
nit: Let's use RUNTIME_ASSERT(it.frame()->type() == StackFrame::JAVA_SCRIPT)

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime.h#...
src/runtime/runtime.h:305: F(GetCallerJSFunction, 0, 1)                \
nit: Can we preserve same order as in the .cc-file, by moving it to the bottom
of the macro?

https://codereview.chromium.org/1146963002/diff/160001/test/mjsunit/get-calle...
File test/mjsunit/get-caller-js-function.js (right):

https://codereview.chromium.org/1146963002/diff/160001/test/mjsunit/get-calle...
test/mjsunit/get-caller-js-function.js:15: }
If we add %OptimizeFunctionOnNextCall(a), could we then get by without having
--always-opt be part of the flags list?

[danno, 2015-06-05 11:49:59]

Feedback addressed, landing

https://codereview.chromium.org/1146963002/diff/160001/src/compiler/js-intrin...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1146963002/diff/160001/src/compiler/js-intrin...
src/compiler/js-intrinsic-lowering.cc:464: if (FLAG_turbo_deoptimization) {
On 2015/06/05 11:21:56, Michael Starzinger wrote:
> This flag no longer exists, we can just perform this step unconditionally now.

Done.

https://codereview.chromium.org/1146963002/diff/160001/src/compiler/js-intrin...
src/compiler/js-intrinsic-lowering.cc:479: // instruction selection, so that
there is no phase-ordering dependency.
On 2015/06/05 11:21:56, Michael Starzinger wrote:
> nit: The TODO now no longer applies, correct?

Sort of. The inlining ordering dependency still exists.

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:424: if (it.frame()->type() != StackFrame::STUB)
{
On 2015/06/05 11:21:56, Michael Starzinger wrote:
> nit: Let's use RUNTIME_ASSERT(it.frame()->type() == StackFrame::STUB)

Done.

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:428: if (it.frame()->type() !=
StackFrame::JAVA_SCRIPT) {
On 2015/06/05 11:21:56, Michael Starzinger wrote:
> nit: Let's use RUNTIME_ASSERT(it.frame()->type() == StackFrame::JAVA_SCRIPT)

Done.

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1146963002/diff/160001/src/runtime/runtime.h#...
src/runtime/runtime.h:305: F(GetCallerJSFunction, 0, 1)                \
On 2015/06/05 11:21:56, Michael Starzinger wrote:
> nit: Can we preserve same order as in the .cc-file, by moving it to the bottom
> of the macro?

Done.

https://codereview.chromium.org/1146963002/diff/160001/test/mjsunit/get-calle...
File test/mjsunit/get-caller-js-function.js (right):

https://codereview.chromium.org/1146963002/diff/160001/test/mjsunit/get-calle...
test/mjsunit/get-caller-js-function.js:15: }
On 2015/06/05 11:21:56, Michael Starzinger wrote:
> If we add %OptimizeFunctionOnNextCall(a), could we then get by without having
> --always-opt be part of the flags list?

Done.

[danno, 2015-06-05 11:50:45]

The CQ bit was checked by danno@chromium.org

[danno, 2015-06-05 11:50:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1146963002/#ps200001
(title: "Review feedback")

[commit-bot: I haz the power, 2015-06-05 11:50:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146963002/200001

[commit-bot: I haz the power, 2015-06-05 12:28:56]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-06-05 12:29:12]

Patchset 11 (id:??) landed as
https://crrev.com/74f9d8c901ba68c2d664b6e1e3ba80cf0efe8e02
Cr-Commit-Position: refs/heads/master@{#28812}