Convert FastNewClosureStub into hydrogen.

Convert FastNewClosureStub into hydrogen.

BUG=
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16356

[mvstanton, 2013-08-07 15:09:31]

PTAL.

Danno, can you pay special attention to the hydrogen.cc changes in Loop and
IfBuilders.

Also, I have a "novel" formatting for this complex hydrogen code in
code-stubs-hydrogen. Curious what you think...
thanks,
--Michael

[Sven Panne, 2013-08-08 06:23:34]

https://codereview.chromium.org/22562002/diff/6001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/22562002/diff/6001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:970: is_optimized.Then();
"Higher order" programming via funny formatting... o_O :-D

Actually this is quite readable, but having to do such funny things suggests
that our builder interfaces are, well, less than ideal... :-P

[mvstanton, 2013-08-09 15:01:33]

Hi guys,
Per our dinner-time conversation of a couple days ago, I removed the
HInnerAllocatedObject hack and made a new hydrogen instruction to contain the
work of storing the code entry in the JSFunction. Thanks,
--Michael

[Michael Starzinger, 2013-08-27 08:12:51]

LGTM, just a couple of nits. Sorry for being so nitty.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:115: void InstallCode(HValue* js_function, HValue*
native_context,
nit: Can we prefix these methods with either "Build" or "Emit" to make it clear
that they don't actually "do what they say" but "emit code that does what they
say"?

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:916: void
CodeStubGraphBuilderBase::InstallCode(HValue* js_function,
suggestion: How about calling this method "EmitInstallOptimizedCode" and the one
below "EmitInstallCode", that would make their relationship clearer.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:929: // No need for write barrier as JSFunction
object is in the new space.
nit: The second sentence of the comment is obsolete. Crankshaft is smart enough
to figure that out on it's own, no need to worry about WBs here.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:955: void
CodeStubGraphBuilderBase::InstallOptimizedClosure(
suggestion: This method could be called "EmitSearchOptimizedCodeMap" or
"EmitInstallFromOptimizedCodeMap", that better describes what it does.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:1055: HInstruction* undefined_value =
Add<HConstant>(factory->undefined_value());
Use graph()->GetConstantUndefined() to get to the "undefined" constant. This
also spares you the trouble of passing it around everywhere.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:1074: // Initialize the rest of the function. We
don't have to update the
nit: The second sentence of the comment is obsolete. Crankshaft is smart enough
to figure that out on it's own, no need to worry about WBs here.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:1082: HInstruction* the_hole =
Add<HConstant>(factory->the_hole_value());
Use graph()->GetConstantHole() here instead.

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3867: // Context::SlotOffset(index)
Instead of the comment, lets add an assert that the result of your computation
is actually correct ...

ASSERT_EQ(offset, Context::SlotOffset(index) + kHeapObjectTag);

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5165: HValue* js_function() { return OperandAt(0); }
nit: s/js_function/function/ ... or ... s/js_function/closure/

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5169: private:
nit: Empty new-line before the "private:" header.

https://codereview.chromium.org/22562002/diff/15001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/22562002/diff/15001/src/runtime.cc#newcode7961
src/runtime.cc:7961: // directly to properties.
nit: Comment does not apply, lets just drop it.

https://codereview.chromium.org/22562002/diff/15001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/22562002/diff/15001/src/runtime.h#newcode398
src/runtime.h:398: F(NewClosureBailout, 1, 1) \
The other runtime functions jumped to by a stub bailout are post-fixed with a
"FromStubFailure" name, I think we should stick to that convention (or
alternatively rename the others as well).

[mvstanton, 2013-08-27 09:23:04]

Nits addressed, thanks they were good ones, esp. those that addressed my rather
muddled function names!

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:115: void InstallCode(HValue* js_function, HValue*
native_context,
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> nit: Can we prefix these methods with either "Build" or "Emit" to make it
clear
> that they don't actually "do what they say" but "emit code that does what they
> say"?

Done.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:916: void
CodeStubGraphBuilderBase::InstallCode(HValue* js_function,
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> suggestion: How about calling this method "EmitInstallOptimizedCode" and the
one
> below "EmitInstallCode", that would make their relationship clearer.

Good idea. Though I use "Build" instead of "Emit," because
CodeStubGraphBuilderBase already has a collection of "Build" helpers for array
functions; it seems more consistent with this file.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:929: // No need for write barrier as JSFunction
object is in the new space.
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> nit: The second sentence of the comment is obsolete. Crankshaft is smart
enough
> to figure that out on it's own, no need to worry about WBs here.

Done.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:955: void
CodeStubGraphBuilderBase::InstallOptimizedClosure(
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> suggestion: This method could be called "EmitSearchOptimizedCodeMap" or
> "EmitInstallFromOptimizedCodeMap", that better describes what it does.

Done.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:1055: HInstruction* undefined_value =
Add<HConstant>(factory->undefined_value());
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> Use graph()->GetConstantUndefined() to get to the "undefined" constant. This
> also spares you the trouble of passing it around everywhere.

Done.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:1074: // Initialize the rest of the function. We
don't have to update the
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> nit: The second sentence of the comment is obsolete. Crankshaft is smart
enough
> to figure that out on it's own, no need to worry about WBs here.

Done.

https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:1082: HInstruction* the_hole =
Add<HConstant>(factory->the_hole_value());
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> Use graph()->GetConstantHole() here instead.

Done.

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3867: // Context::SlotOffset(index)
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> Instead of the comment, lets add an assert that the result of your computation
> is actually correct ...
> 
> ASSERT_EQ(offset, Context::SlotOffset(index) + kHeapObjectTag);

Done.

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5165: HValue* js_function() { return OperandAt(0); }
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> nit: s/js_function/function/ ... or ... s/js_function/closure/

Done.

https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5169: private:
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> nit: Empty new-line before the "private:" header.

Done.

https://codereview.chromium.org/22562002/diff/15001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/22562002/diff/15001/src/ia32/lithium-ia32.h#n...
src/ia32/lithium-ia32.h:1788: LOperand* js_function() { return inputs_[0]; }
I continued the 's/js_function/function' work into here.

https://codereview.chromium.org/22562002/diff/15001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/22562002/diff/15001/src/runtime.cc#newcode7961
src/runtime.cc:7961: // directly to properties.
On 2013/08/27 08:12:51, Michael Starzinger wrote:
> nit: Comment does not apply, lets just drop it.

Done.

[mvstanton, 2013-08-27 11:55:41]

Committed patchset #6 manually as r16356 (presubmit successful).