Add a MathFloor stub generated with TurboFan

Add a MathFloor stub generated with TurboFan

This stub will be used as the basis of a Math.floor-specific CallIC to
detect and track calls to floor that return -0.

Along the way:
- Create a TurboFanCodeStub super class from which the StringLength and
MathRound TF stubs derive.
- Fix the ugly hack that passes the first stub parameter as the "this"
pointer in the the TF-compiled JS function.
- Fix bugs in the ia32/x64 disassembler.

Committed: https://crrev.com/abc35080b33cc31e47b9913e64ac99ec687c6b65
Cr-Commit-Position: refs/heads/master@{#28339}

[danno, 2015-05-08 11:55:23]

danno@chromium.org changed reviewers:
+ bmeurer@chromium.org, mvstanton@chromium.org

[danno, 2015-05-08 11:56:01]

PTAL

[Benedikt Meurer, 2015-05-08 12:17:07]

First round of comments.

https://codereview.chromium.org/1137703002/diff/60001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1137703002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:462: static Handle<JSFunction> GetFunction(Isolate* isolate,
const char* name) {
Nit: no static inside anonymous namespace.

https://codereview.chromium.org/1137703002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:476: };
Remove ; and add empty line before, plus add // namespace comment to make
cpplint and clang-format happy

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/access-bui...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/access-bui...
src/compiler/access-builder.h:69: static FieldAccess ForSharedFunctionInfo();
I think the naming scheme suggests ForJSFunctionSharedFunctionInfo().

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/access-bui...
src/compiler/access-builder.h:72: static FieldAccess ForTypeFeedbackVector();
ForSharedFunctionInfoTypeFeedbackVector()

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

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:200:
node->set_op(machine()->Word32Equal());
How about

 return Change(node, machine()->Word32Equal(), and_result,
jsgraph()->Int32Constant(1))

here?

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:474: Reduction
JSIntrinsicLowering::Change(Node* node, const Operator* op, Node* a,
We don't need this helper?

https://codereview.chromium.org/1137703002/diff/60001/test/mjsunit/tf-stubs-f...
File test/mjsunit/tf-stubs-floor.js (right):

https://codereview.chromium.org/1137703002/diff/60001/test/mjsunit/tf-stubs-f...
test/mjsunit/tf-stubs-floor.js:28: // Flags: --allow-natives-syntax
--expose-natives-as=builtins --noalways-opt
Please move to some subdirectory of test/mjsunit/compiler, i.e.
test/mjsunit/compiler/stubs

[danno, 2015-05-08 15:28:01]

Please take another look

https://codereview.chromium.org/1137703002/diff/60001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1137703002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:462: static Handle<JSFunction> GetFunction(Isolate* isolate,
const char* name) {
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> Nit: no static inside anonymous namespace.

Done.

https://codereview.chromium.org/1137703002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:476: };
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> Remove ; and add empty line before, plus add // namespace comment to make
> cpplint and clang-format happy

Done.

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/access-bui...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/access-bui...
src/compiler/access-builder.h:69: static FieldAccess ForSharedFunctionInfo();
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> I think the naming scheme suggests ForJSFunctionSharedFunctionInfo().

Done.

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/access-bui...
src/compiler/access-builder.h:72: static FieldAccess ForTypeFeedbackVector();
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> ForSharedFunctionInfoTypeFeedbackVector()

Done.

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

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:200:
node->set_op(machine()->Word32Equal());
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> How about
> 
>  return Change(node, machine()->Word32Equal(), and_result,
> jsgraph()->Int32Constant(1))
> 
> here?

Done.

https://codereview.chromium.org/1137703002/diff/60001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:474: Reduction
JSIntrinsicLowering::Change(Node* node, const Operator* op, Node* a,
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> We don't need this helper?

Done.

https://codereview.chromium.org/1137703002/diff/60001/test/mjsunit/tf-stubs-f...
File test/mjsunit/tf-stubs-floor.js (right):

https://codereview.chromium.org/1137703002/diff/60001/test/mjsunit/tf-stubs-f...
test/mjsunit/tf-stubs-floor.js:28: // Flags: --allow-natives-syntax
--expose-natives-as=builtins --noalways-opt
On 2015/05/08 12:17:07, Benedikt Meurer wrote:
> Please move to some subdirectory of test/mjsunit/compiler, i.e.
> test/mjsunit/compiler/stubs

Done.

[Benedikt Meurer, 2015-05-11 04:40:12]

https://codereview.chromium.org/1137703002/diff/100001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1137703002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:19: using namespace v8::internal::compiler;
This violates the style guide. No toplevel using namespace. How about moving
this into the GenerateCode method below? Or even better: Move the last three
lines of GenerateCode into a method on the CompilationInfo, i.e.
GenerateCodeStub().

https://codereview.chromium.org/1137703002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:476: }
Use

}  // namespace

instead of just

}

to make both clang-format and cpplint agree :-)

https://codereview.chromium.org/1137703002/diff/100001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/1137703002/diff/100001/src/compiler/access-bu...
src/compiler/access-builder.cc:161: FieldAccess
AccessBuilder::ForSharedFunctionInfoTypeFeedbackVector() {
Nit: Missing // static before definition.

https://codereview.chromium.org/1137703002/diff/100001/src/compiler/access-bu...
src/compiler/access-builder.cc:165: 
Nit: redundant new line :-)

https://codereview.chromium.org/1137703002/diff/100001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1137703002/diff/100001/src/runtime.js#newcode906
src/runtime.js:906: %_FixedArraySet(%_GetTypeFeedbackVector(f), ((i|0)+1)|0, 1);
Please add a comment explaining what this line does.

https://codereview.chromium.org/1137703002/diff/100001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/1137703002/diff/100001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:76: RUNTIME_FUNCTION(Runtime_GetTypeFeedbackVector)
{
runtime-array.cc doesn't seems to be a good candidate for this function...

[danno, 2015-05-11 06:14:19]

Feedback addressed. Please take another look

https://codereview.chromium.org/1137703002/diff/100001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1137703002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:19: using namespace v8::internal::compiler;
On 2015/05/11 04:40:12, Benedikt Meurer wrote:
> This violates the style guide. No toplevel using namespace. How about moving
> this into the GenerateCode method below? Or even better: Move the last three
> lines of GenerateCode into a method on the CompilationInfo, i.e.
> GenerateCodeStub().

Done.

https://codereview.chromium.org/1137703002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:476: }
On 2015/05/11 04:40:12, Benedikt Meurer wrote:
> Use
> 
> }  // namespace
> 
> instead of just
> 
> }
> 
> to make both clang-format and cpplint agree :-)

Done.

https://codereview.chromium.org/1137703002/diff/100001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/1137703002/diff/100001/src/compiler/access-bu...
src/compiler/access-builder.cc:161: FieldAccess
AccessBuilder::ForSharedFunctionInfoTypeFeedbackVector() {
On 2015/05/11 04:40:12, Benedikt Meurer wrote:
> Nit: Missing // static before definition.

Done.

https://codereview.chromium.org/1137703002/diff/100001/src/compiler/access-bu...
src/compiler/access-builder.cc:165: 
On 2015/05/11 04:40:12, Benedikt Meurer wrote:
> Nit: redundant new line :-)

Done.

https://codereview.chromium.org/1137703002/diff/100001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1137703002/diff/100001/src/runtime.js#newcode906
src/runtime.js:906: %_FixedArraySet(%_GetTypeFeedbackVector(f), ((i|0)+1)|0, 1);
On 2015/05/11 04:40:12, Benedikt Meurer wrote:
> Please add a comment explaining what this line does. 

Done.

https://codereview.chromium.org/1137703002/diff/100001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/1137703002/diff/100001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:76: RUNTIME_FUNCTION(Runtime_GetTypeFeedbackVector)
{
On 2015/05/11 04:40:12, Benedikt Meurer wrote:
> runtime-array.cc doesn't seems to be a good candidate for this function...

Done.

[Benedikt Meurer, 2015-05-11 06:16:35]

Thanks. LGTM

[danno, 2015-05-11 07:17:24]

The CQ bit was checked by danno@chromium.org

[danno, 2015-05-11 07:17:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1137703002/#ps130001
(title: "Small cleanup")

[commit-bot: I haz the power, 2015-05-11 07:17:55]

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

[commit-bot: I haz the power, 2015-05-11 07:20:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 07:20:35]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/257)

[danno, 2015-05-11 09:00:04]

The CQ bit was checked by danno@chromium.org

[danno, 2015-05-11 09:00:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1137703002/#ps190001
(title: "More merge fun")

[commit-bot: I haz the power, 2015-05-11 09:00:23]

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

[commit-bot: I haz the power, 2015-05-11 09:09:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 09:09:16]

Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[danno, 2015-05-11 09:14:26]

The CQ bit was checked by danno@chromium.org

[danno, 2015-05-11 09:14:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1137703002/#ps210001
(title: "Fix pedantic duplicate ; detection")

[commit-bot: I haz the power, 2015-05-11 09:14:53]

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

[commit-bot: I haz the power, 2015-05-11 09:38:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 09:38:43]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/5584)

[danno, 2015-05-11 11:21:12]

The CQ bit was checked by danno@chromium.org

[danno, 2015-05-11 11:21:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1137703002/#ps230001
(title: "Fix ARM + co.")

[commit-bot: I haz the power, 2015-05-11 11:21:25]

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

[commit-bot: I haz the power, 2015-05-11 11:45:09]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-05-11 11:45:23]

Patchset 13 (id:??) landed as
https://crrev.com/abc35080b33cc31e47b9913e64ac99ec687c6b65
Cr-Commit-Position: refs/heads/master@{#28339}