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

src/ia32/full-codegen-ia32.cc

Index: src/ia32/full-codegen-ia32.cc
===================================================================
--- src/ia32/full-codegen-ia32.cc	(revision 7676)
+++ src/ia32/full-codegen-ia32.cc	(working copy)
@@ -3725,48 +3725,13 @@
       break;
     }
 
-    case Token::SUB: {
-      Comment cmt(masm_, "[ UnaryOperation (SUB)");
-      bool can_overwrite = expr->expression()->ResultOverwriteAllowed();
-      UnaryOverwriteMode overwrite =
-          can_overwrite ? UNARY_OVERWRITE : UNARY_NO_OVERWRITE;
-      GenericUnaryOpStub stub(Token::SUB, overwrite, NO_UNARY_FLAGS);
-      // GenericUnaryOpStub expects the argument to be in the
-      // accumulator register eax.
-      VisitForAccumulatorValue(expr->expression());
-      __ CallStub(&stub);
-      context()->Plug(eax);
+    case Token::SUB:
+      EmitUnaryOperation(expr, "[ UnaryOperation (SUB)");
       break;
-    }
 
-    case Token::BIT_NOT: {
-      Comment cmt(masm_, "[ UnaryOperation (BIT_NOT)");
-      // The generic unary operation stub expects the argument to be
-      // in the accumulator register eax.
-      VisitForAccumulatorValue(expr->expression());
-      Label done;
-      bool inline_smi_case = ShouldInlineSmiCase(expr->op());
-      if (inline_smi_case) {
-        NearLabel call_stub;
-        __ test(eax, Immediate(kSmiTagMask));
-        __ j(not_zero, &call_stub);
-        __ lea(eax, Operand(eax, kSmiTagMask));
-        __ not_(eax);
-        __ jmp(&done);
-        __ bind(&call_stub);
-      }
-      bool overwrite = expr->expression()->ResultOverwriteAllowed();
-      UnaryOverwriteMode mode =
-          overwrite ? UNARY_OVERWRITE : UNARY_NO_OVERWRITE;
-      UnaryOpFlags flags = inline_smi_case
-          ? NO_UNARY_SMI_CODE_IN_STUB
-          : NO_UNARY_FLAGS;
-      GenericUnaryOpStub stub(Token::BIT_NOT, mode, flags);
-      __ CallStub(&stub);
-      __ bind(&done);
-      context()->Plug(eax);
+    case Token::BIT_NOT:
+      EmitUnaryOperation(expr, "[ UnaryOperation (BIT_NOT)");
       break;
-    }
 
     default:
       UNREACHABLE();
@@ -3774,6 +3739,22 @@
 }
 
 
+void FullCodeGenerator::EmitUnaryOperation(UnaryOperation* expr,
+                                           const char* comment) {
+  // TODO(svenpanne): Allowing format strings in Comment would be nice here...

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

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.

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

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?

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

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

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

OK, I'll have a look, especially if the change is really worth it, but this
would be in another change, anyway.

+  Comment cmt(masm_, comment);
+  bool can_overwrite = expr->expression()->ResultOverwriteAllowed();
+  UnaryOverwriteMode overwrite =
+      can_overwrite ? UNARY_OVERWRITE : UNARY_NO_OVERWRITE;
+  TypeRecordingUnaryOpStub stub(expr->op(), overwrite);
+  // TypeRecordingUnaryOpStub expects the argument to be in the
+  // accumulator register eax.
+  VisitForAccumulatorValue(expr->expression());

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

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.

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

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.

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

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.

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

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. :-)

+  __ CallStub(&stub);

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

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)

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

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...

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

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.

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

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

+  context()->Plug(eax);
+}
+
+
 void FullCodeGenerator::VisitCountOperation(CountOperation* expr) {
   Comment cmnt(masm_, "[ CountOperation");
   SetSourcePosition(expr->position());