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

src/ia32/full-codegen-ia32.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 3707 matching lines @@
       Label no_conversion;
       __ test(result_register(), Immediate(kSmiTagMask));
       __ j(zero, &no_conversion);
       ToNumberStub convert_stub;
       __ CallStub(&convert_stub);
       __ bind(&no_conversion);
       context()->Plug(result_register());
       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();
   }
 }
 
 
+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());
 
   // Invalid left-hand sides are rewritten to have a 'throw ReferenceError'
   // as the left-hand side.
   if (!expr->expression()->IsValidLeftHandSide()) {
     VisitForEffect(expr->expression());
     return;
   }
@@ skipping 536 matching lines @@
   // And return.
   __ ret(0);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_IA32