Added type recording for unary minus and unary bitwise negation. Note that the

Added type recording for unary minus and unary bitwise negation. Note that the
type info itself is not used in crankshaft itself (yet).

BUG=v8:1311

[Erik Corry, 2011-04-20 19:13:30]

Drive-by.

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1983: if (!mode_ == UNARY_OVERWRITE) {
!mode_ == should be mode_ !=

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1987: __ AllocateHeapNumber(r2, r3, r4, r6, slow);
This will generate a type transition on a GC.  That gives wrong type feedback to
the Cranker because it goes generic.

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2003: __ Call(stub.GetCode(), RelocInfo::CODE_TARGET);
tail call

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2004: __ pop(lr);
Instead of push-call-pop you can use a tail call ie a jump.

[Sven Panne, 2011-04-20 20:27:50]

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1983: if (!mode_ == UNARY_OVERWRITE) {
On 2011/04/20 19:13:30, Erik Corry wrote:
> !mode_ == should be mode_ !=

... or even clearer: mode == UNARY_NO_OVERWRITE (Uncle Bob says: Don't use
negation ;-) My code was based on GenericUnaryOpStub::Generate, which has a
similar comparison, but I guess that GenericUnaryOpStub can be nuked soon,
anyway.

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1987: __ AllocateHeapNumber(r2, r3, r4, r6, slow);
On 2011/04/20 19:13:30, Erik Corry wrote:
> This will generate a type transition on a GC.  That gives wrong type feedback
to
> the Cranker because it goes generic.

I don't understand that one. Could you elaborate, please?

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2003: __ Call(stub.GetCode(), RelocInfo::CODE_TARGET);
On 2011/04/20 19:13:30, Erik Corry wrote:
> tail call

Done.

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2004: __ pop(lr);
On 2011/04/20 19:13:30, Erik Corry wrote:
> Instead of push-call-pop you can use a tail call ie a jump.

I guess that this means simply using __ Jump(...) here. For clarity, I've moved
the Ret into the if branch where it belongs then.

[Erik Corry, 2011-04-21 02:48:57]

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1983: if (!mode_ == UNARY_OVERWRITE) {
On 2011/04/20 20:27:50, Sven wrote:
> On 2011/04/20 19:13:30, Erik Corry wrote:
> > !mode_ == should be mode_ !=
> 
> ... or even clearer: mode == UNARY_NO_OVERWRITE (Uncle Bob says: Don't use
> negation ;-) My code was based on GenericUnaryOpStub::Generate, which has a
> similar comparison, but I guess that GenericUnaryOpStub can be nuked soon,
> anyway.

Silly C++ type system lets you do ! to an enum :-(

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1987: __ AllocateHeapNumber(r2, r3, r4, r6, slow);
On 2011/04/20 20:27:50, Sven wrote:
> On 2011/04/20 19:13:30, Erik Corry wrote:
> > This will generate a type transition on a GC.  That gives wrong type
feedback
> to
> > the Cranker because it goes generic.
> 
> I don't understand that one. Could you elaborate, please?

If the stub encounters something that is not a number on the input side then it
should transition the stub call site to the generic state. This tells a later
Crankshaft code generation routine to call the stub at this point since the
inlined Crankshaft code can only cope with numbers and would cause an expensive
deopt if another nonnumber came along.

On the other hand a failure to bump-allocate a heap number doesn't tell us
anything that we need to communicate to Crankshaft.  It just means that we
coincidentally hit the end of newspace here.  In this case we need to cause a GC
and return the correct answer, but we don't need to transition the stub call
site into the generic state.  Going generic would cause needlessly suboptimal
code to be generated by Crankshaft.

Perhaps this flaw is shared by the binary version?

There is a runtime call that merely allocates a heap number, possibly doing a
GC.  You can use that.

[Sven Panne, 2011-04-21 05:12:18]

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1987: __ AllocateHeapNumber(r2, r3, r4, r6, slow);
On 2011/04/21 02:48:57, Erik Corry wrote:
> [...] On the other hand a failure to bump-allocate a heap number doesn't tell
us
> anything that we need to communicate to Crankshaft.  It just means that we
> coincidentally hit the end of newspace here.  In this case we need to cause a
GC
> and return the correct answer, but we don't need to transition the stub call
> site into the generic state.  Going generic would cause needlessly suboptimal
> code to be generated by Crankshaft.

OK, understood.

> Perhaps this flaw is shared by the binary version?

Nope, but it is shared by my unary minus implementation, too, of course on all 3
platforms... :-P

> There is a runtime call that merely allocates a heap number, possibly doing a
> GC.  You can use that.

The existing code is a bit hard to grasp because there is a plethora of
allocation functions available. Furthermore, I can't find a documentation which
function/runtime call/builtin uses/clobbers which register. In addition, code
generation is complicated by the fact that a full nursery "shines through" via
gc_required, something I believe can be avoided. Nevertheless, I'll give it a
try... :-)

[Sven Panne, 2011-04-21 05:21:41]

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879081/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1987: __ AllocateHeapNumber(r2, r3, r4, r6, slow);
Before I continue to fix the other occurences of AllocateHeapNumber in my patch,
I'd like to hear comments on this concrete one (unary not on ARM), because the
AllocateHeapNumber/CallRuntime combo looks a bit complicated...

[Sven Panne, 2011-04-21 16:19:12]

http://codereview.chromium.org/6879081/diff/1024/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879081/diff/1024/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:1988: __ bind(&slow_allocate_heapnumber);
Hmmm, this seems to crash mjsunit/bit-not.js with the following message:

---------------------------------------------------
#
# Fatal error in /home/svenpanne/v8/src/frames.cc, line 429
# CHECK(kind == Code::FUNCTION || kind == Code::OPTIMIZED_FUNCTION) failed
#



Attempt to print stack while printing stack (double fault)
If you are lucky you may find a partial stack dump on stdout.
---------------------------------------------------

It seems that I've violated some implicit global assumption, but I've got no
clue what it is. Hints appreciated...

[Erik Corry, 2011-04-22 18:45:09]

On 2011/04/21 16:19:12, Sven wrote:
> http://codereview.chromium.org/6879081/diff/1024/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/6879081/diff/1024/src/arm/code-stubs-arm.cc#ne...
> src/arm/code-stubs-arm.cc:1988: __ bind(&slow_allocate_heapnumber);
> Hmmm, this seems to crash mjsunit/bit-not.js with the following message:
> 
> ---------------------------------------------------
> #
> # Fatal error in /home/svenpanne/v8/src/frames.cc, line 429
> # CHECK(kind == Code::FUNCTION || kind == Code::OPTIMIZED_FUNCTION) failed
> #
> 
> 
> 
> Attempt to print stack while printing stack (double fault)
> If you are lucky you may find a partial stack dump on stdout.
> ---------------------------------------------------
> 
> It seems that I've violated some implicit global assumption, but I've got no
> clue what it is. Hints appreciated...

When you make a call you need to either return before the next GC or you need to
set up a frame so that the GC can traverse the stack.  There is support in the
macro assembler for setting up the frame.  If you already set up a frame then
you can often get away with a tail call which reuses the frame.  If you call
something that both does the allocation and calculates the unary operation then
you can do a tail call.  I think that is what the state transition call does. 
It also transitions the stub call site state which you don't want in this case.

[fschneider, 2011-04-26 13:32:52]

The problem is that you're missing recording of the source position in the
full-codegen. But it is easy to fix. I sent a few small comments on the code
review as well.

Until now we had no need for that, since the old UnaryOpStub was not an IC - the
type oracle iterates over all calls to IC and has the implicit assumption that
there is a source position recorded for each IC call. If not, things go wrong -
i.e. it will record two ICs for the same source position etc. resulting in
missing typefeedback for some operations.

You need to add a position_ member to the UnaryOperation class in ast.h. In
parser.cc you need to pass the position into the constructor of UnaryOperation
(like we do e.g. for BinaryOperation)

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:719: 
Remove one extra line.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.h
File src/ia32/code-stubs-ia32.h (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.h#...
src/ia32/code-stubs-ia32.h:143: void GenerateHeapNumberCodeSub(MacroAssembler*
masm, // Label* undo,
Remove commented code + fit on one line.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.h#...
src/ia32/code-stubs-ia32.h:145: void
GenerateHeapNumberCodeBitNot(MacroAssembler* masm, // Label* undo,
Remove commented code + fit on one line.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3744: // TODO(svenpanne): Allowing format strings
in Comment would be nice here...
It's a little complicated to do, since format strings require allocating a
buffer in JS heap since comments are referenced by the reloc-info... - so I'm
not sure if you should make it a TODO now.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3752:
VisitForAccumulatorValue(expr->expression());
You need to call here:

SetSourcePosition(expr->position())

This will record the statement position which we use to associate the type
feedback with. We have the same call to SetSourcePosition for binary operations.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3753: __ CallStub(&stub);
You should use here:

EmitCallIC(stub.GetCode(), NULL)

where NULL signals that there is no inlined smi code.
Since we probably want to add patchable inline smi code later (like we already
do for binary ops)

http://codereview.chromium.org/6879081/diff/10001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ic.cc#newcode1819
src/ic.cc:1819: case SMI: return "SMI";
--> "Smi"

[Sven Panne, 2011-04-27 00:45:54]

After some testing, I'll upload a new version with the changes...

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:719: 
On 2011/04/26 13:32:52, fschneider wrote:
> Remove one extra line.

Done.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.h
File src/ia32/code-stubs-ia32.h (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.h#...
src/ia32/code-stubs-ia32.h:143: void GenerateHeapNumberCodeSub(MacroAssembler*
masm, // Label* undo,
On 2011/04/26 13:32:52, fschneider wrote:
> Remove commented code + fit on one line.

Done.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/code-stubs-ia32.h#...
src/ia32/code-stubs-ia32.h:145: void
GenerateHeapNumberCodeBitNot(MacroAssembler* masm, // Label* undo,
On 2011/04/26 13:32:52, fschneider wrote:
> Remove commented code + fit on one line.

Done.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3744: // TODO(svenpanne): Allowing format strings
in Comment would be nice here...
On 2011/04/26 13:32:52, fschneider wrote:
> It's a little complicated to do, since format strings require allocating a
> buffer in JS heap since comments are referenced by the reloc-info... - so I'm
> not sure if you should make it a TODO now.

I thought about using something like vsnprintf in the Comment constructor, this
shouldn't be too hard, I think or am I missing something here?

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3752:
VisitForAccumulatorValue(expr->expression());
On 2011/04/26 13:32:52, fschneider wrote:
> You need to call here:
> 
> SetSourcePosition(expr->position())
> 
> This will record the statement position which we use to associate the type
> feedback with. We have the same call to SetSourcePosition for binary
operations.

Done, for x64 and ARM, too.

Why is Expression::position() not pure? Using UNREACHABLE just hides semantic
errors when a subclass doesn't properly implement this. Another possibility
might be moving the pos_ field to Expression itself, making position() a
"normal" method there.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3753: __ CallStub(&stub);
On 2011/04/26 13:32:52, fschneider wrote:
> You should use here:
> 
> EmitCallIC(stub.GetCode(), NULL)
> 
> where NULL signals that there is no inlined smi code.
> Since we probably want to add patchable inline smi code later (like we already
> do for binary ops)

I have to admit that I still don't understand this kind of patching in detail,
but I've changed this like you said. :-) Of course, the x64 and ARM parts have
been changed accordingly, too. Note that IMHO this triple change is a strong
hint that there is a bit too much copy-n-paste in the code...

http://codereview.chromium.org/6879081/diff/10001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ic.cc#newcode1819
src/ic.cc:1819: case SMI: return "SMI";
On 2011/04/26 13:32:52, fschneider wrote:
> --> "Smi"

Done.

[fschneider, 2011-04-27 11:34:36]

Looks pretty good now.

LGTM with some small comments.

I'm also fine if you want to address some as a separate change, if this cl is
getting too large.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3744: // TODO(svenpanne): Allowing format strings
in Comment would be nice here...
On 2011/04/27 00:45:54, Sven wrote:
> On 2011/04/26 13:32:52, fschneider wrote:
> > It's a little complicated to do, since format strings require allocating a
> > buffer in JS heap since comments are referenced by the reloc-info... - so
I'm
> > not sure if you should make it a TODO now.
> 
> I thought about using something like vsnprintf in the Comment constructor,
this
> shouldn't be too hard, I think or am I missing something here?

As Kevin said, we do something similar in Crankshaft (lithium-*) where we print
the Lithium-IR as comments.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3752:
VisitForAccumulatorValue(expr->expression());
On 2011/04/27 00:45:54, Sven wrote:
> On 2011/04/26 13:32:52, fschneider wrote:
> > You need to call here:
> > 
> > SetSourcePosition(expr->position())
> > 
> > This will record the statement position which we use to associate the type
> > feedback with. We have the same call to SetSourcePosition for binary
> operations.
> 
> Done, for x64 and ARM, too.
> 
> Why is Expression::position() not pure? Using UNREACHABLE just hides semantic
> errors when a subclass doesn't properly implement this. Another possibility
> might be moving the pos_ field to Expression itself, making position() a
> "normal" method there.

I agree that it would be a good idea to make it abstract. We don't want to store
positions for AST nodes where it is not needed however. Feel free to do it as a
separate change - otherwise this changelist would grow too large.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3753: __ CallStub(&stub);
On 2011/04/27 00:45:54, Sven wrote:
> On 2011/04/26 13:32:52, fschneider wrote:
> > You should use here:
> > 
> > EmitCallIC(stub.GetCode(), NULL)
> > 
> > where NULL signals that there is no inlined smi code.
> > Since we probably want to add patchable inline smi code later (like we
already
> > do for binary ops)
> 
> I have to admit that I still don't understand this kind of patching in detail,
> but I've changed this like you said. :-) Of course, the x64 and ARM parts have
> been changed accordingly, too. Note that IMHO this triple change is a strong
> hint that there is a bit too much copy-n-paste in the code...

Have a look at uses of JumpPatchSite for BinaryOperation. It's an abstraction
for a conditional branch that gets patched on the first execution of the IC
stub. 

It's probably good to add inlined smi code for SUB and BIT_NOT as a separate
change.

http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (left):

http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3733: GenericUnaryOpStub stub(Token::SUB,
overwrite, NO_UNARY_FLAGS);
Please remove the code for the GenericUnaryOpStub (in code-stubs.h, etc.), since
it is not used anymore after your change.

http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:455: void
TypeRecordingUnaryOpStub::GenerateSmiStubSub(MacroAssembler* masm) {
I'm not convinced that all the short helpers like this one, that are used
exactly in one place, buy us much.

If you plan to refactor these helpers anyway, it's ok to do this as a separate
change later.

[Sven Panne, 2011-04-27 17:19:31]

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3744: // TODO(svenpanne): Allowing format strings
in Comment would be nice here...
OK, I'll have a look, especially if the change is really worth it, but this
would be in another change, anyway.

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3752:
VisitForAccumulatorValue(expr->expression());
I'll investigate this later, and I wanted to do this in a separate change (if
any), anyway. Is the AST really kept around for an extended period of time? If
not, pulling the position into Expression (or even higher in the class
hierarchy) seems to be the the right thing. Even if the position is not used for
some kind of nodes, I like simple rules like "every expression has a position"
instead of having crankshaft silently generate obscure code. :-)

http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3753: __ CallStub(&stub);
On 2011/04/27 11:34:36, fschneider wrote:
> [...] It's probably good to add inlined smi code for SUB and BIT_NOT as a
separate change.

OK, I'll keep this in mind...

http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (left):

http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:3733: GenericUnaryOpStub stub(Token::SUB,
overwrite, NO_UNARY_FLAGS);
On 2011/04/27 11:34:36, fschneider wrote:
> Please remove the code for the GenericUnaryOpStub (in code-stubs.h, etc.),
since it is not used anymore after your change.

I already have exactly this on my TODO list, plus a few other minor cleanups. I
want to do each cleanup in a small, separate change to avoid monster commits
like this one.

http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:455: void
TypeRecordingUnaryOpStub::GenerateSmiStubSub(MacroAssembler* masm) {
On 2011/04/27 11:34:36, fschneider wrote:
> I'm not convinced that all the short helpers like this one, that are used
exactly in one place, buy us much.

Perhaps they are a little bit small, but they avoid copy-n-paste of machine code
generation, and you can easily see the structure of the stubs.

> If you plan to refactor these helpers anyway, it's ok to do this as a separate
change later.

I have some ideas here, let's see if there's time...