Refine CountOperation of FullCodeGen

src/x64/full-codegen-x64.cc

 // Copyright 2012 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 4377 matching lines @@
   }
 
   // We need a second deoptimization point after loading the value
   // in case evaluating the property load my have a side effect.
   if (assign_type == VARIABLE) {
     PrepareForBailout(expr->expression(), TOS_REG);
   } else {
     PrepareForBailoutForId(prop->LoadId(), TOS_REG);
   }
 
-  // Call ToNumber only if operand is not a smi.
-  Label no_conversion;
+  // Inline smi case if we are in a loop.
+  Label done, stub_call;
+  JumpPatchSite patch_site(masm_);
   if (ShouldInlineSmiCase(expr->op())) {
-    __ JumpIfSmi(rax, &no_conversion, Label::kNear);
+    Label slow;
+    patch_site.EmitJumpIfNotSmi(rax, &slow, Label::kNear);
+
+    // Save result for postfix expressions.
+    if (expr->is_postfix()) {
+      if (!context()->IsEffect()) {
+        // Save the result on the stack. If we have a named or keyed property
+        // we store the result under the receiver that is currently on top
+        // of the stack.
+        switch (assign_type) {
+          case VARIABLE:
+            __ push(rax);
+            break;
+          case NAMED_PROPERTY:
+            __ movq(Operand(rsp, kPointerSize), rax);
+            break;
+          case KEYED_PROPERTY:
+            __ movq(Operand(rsp, 2 * kPointerSize), rax);
+            break;
+        }
+      }
+    }
+
+    if (expr->op() == Token::INC) {

[haitao.feng, 2013/08/13 09:05:57]

An alternative is:
    if (expr->op() == Token::INC) {
      __ SmiAddConstant(rax, rax, Smi::FromInt(1), &stub_call);
    } else {
      __ SmiSubConstant(rax, rax, Smi::FromInt(1), &stub_call);
    }
    __ jmp(&done, Label::kNear);
    __ bind(&slow);

This version uses 5 instructions in SmiAddConstant (movq kScratchRegister,
kSmiConstantRegister; addq(kScratchRegister, rax); j(overflow, &stub_call);
movq(rax, kScratchRegister), jmp(&done)) than 2 (addq(rax,
kSmiConstantRegister), j(no_overflow, &done) for normal cases.

[danno, 2013/08/19 19:36:58]

I think the shorter sequence is better without the scratch register in the case
that performance matters.

Have you considered making the SmiAddConstant/SmiSubConstant handle the overflow
case similarly to how this code does? e.g.

SmiAddConstant(...) {
addq(...)
Label local_done
j(no_overflow, &local_done);
subq(...)
jmp(&stub)
bind(&local_done);
}

Or do the extra branches destroy performance in this case? 

Perhaps you should leave the "fast" version for now and we'll tackle this
problem in the CL that introduces the SmiWrapper

On 2013/08/13 09:05:57, haitao.feng wrote:

[haitao.feng, 2013/08/20 14:36:05]

Thanks for the review and recommendation. I will make this CL an incremental
step of introducing the SmiWrapper.

How about the other part of this CL? i.e., guiding the SmiAddConstant and
SmiSubConstant by JumpIfNotSmi? Do we need to address the issue of using
SmiAddConstant for both SMI and HeapNumber?

[danno, 2013/08/20 15:39:37]

I like your current approach where the Smi/no Smi decision is made very early,
that looks good. The "heap number optimization" in the old code was (as you
point out) subtle and it's unclear to me that the old version is actually a
performance win. It saved on code size, but without concrete performance numbers
to show that it was better your new, explicit split of smi/heap code number
early-on seems c cleaner and easier to understand.

I think there might even be a bug in the old code that you have fixed!
Previously, if there was an overflow in the smi constant add/sub case, we'd jump
to the stub... but not restore rax to it's pre-increment state. The stub would
work on the post-incremented value, not the pre-incremented value!

[haitao.feng, 2013/08/20 23:21:47]

Thanks for the comments. Then I will port it to IA32 and ARM. Please note that
"Save result for postfix expressions" are duplicated in the code because of this
early split. I did not add a sub function in FullCodeGen for it as it is small.

There is no bug in the original code, the code after the label stub_call will
restore rax first and then call the stub function. This code is quite special
and different with any other SMI function usage.

+      __ SmiAddConstant(rax, rax, Smi::FromInt(1));
+    } else {
+      __ SmiSubConstant(rax, rax, Smi::FromInt(1));
+    }
+    __ j(no_overflow, &done, Label::kNear);
+    // Call stub. Undo operation first.
+    if (expr->op() == Token::INC) {
+      __ SmiSubConstant(rax, rax, Smi::FromInt(1));
+    } else {
+      __ SmiAddConstant(rax, rax, Smi::FromInt(1));
+    }
+    __ jmp(&stub_call, Label::kNear);
+    __ bind(&slow);
   }
+
   ToNumberStub convert_stub;
   __ CallStub(&convert_stub);
-  __ bind(&no_conversion);
 
   // Save result for postfix expressions.
   if (expr->is_postfix()) {
     if (!context()->IsEffect()) {
       // Save the result on the stack. If we have a named or keyed property
       // we store the result under the receiver that is currently on top
       // of the stack.
       switch (assign_type) {
         case VARIABLE:
           __ push(rax);
           break;
         case NAMED_PROPERTY:
           __ movq(Operand(rsp, kPointerSize), rax);
           break;
         case KEYED_PROPERTY:
           __ movq(Operand(rsp, 2 * kPointerSize), rax);
           break;
       }
     }
   }
 
-  // Inline smi case if we are in a loop.
-  Label done, stub_call;
-  JumpPatchSite patch_site(masm_);
-
-  if (ShouldInlineSmiCase(expr->op())) {
-    if (expr->op() == Token::INC) {
-      __ SmiAddConstant(rax, rax, Smi::FromInt(1));

[haitao.feng, 2013/08/13 09:05:57]

It takes me some time to understand that the original code uses the
SmiAddConstant for both SMI and HeapNumber.

-    } else {
-      __ SmiSubConstant(rax, rax, Smi::FromInt(1));
-    }
-    __ j(overflow, &stub_call, Label::kNear);
-    // We could eliminate this smi check if we split the code at
-    // the first smi check before calling ToNumber.

[haitao.feng, 2013/08/13 09:05:57]

This Cl splits the code at the first smi check.

-    patch_site.EmitJumpIfSmi(rax, &done, Label::kNear);
-
-    __ bind(&stub_call);
-    // Call stub. Undo operation first.
-    if (expr->op() == Token::INC) {
-      __ SmiSubConstant(rax, rax, Smi::FromInt(1));
-    } else {
-      __ SmiAddConstant(rax, rax, Smi::FromInt(1));
-    }
-  }
-
   // Record position before stub call.
   SetSourcePosition(expr->position());
 
   // Call stub for +1/-1.
+  __ bind(&stub_call);
   __ movq(rdx, rax);
   __ Move(rax, Smi::FromInt(1));
   BinaryOpStub stub(expr->binary_op(), NO_OVERWRITE);
   CallIC(stub.GetCode(isolate()),
          RelocInfo::CODE_TARGET,
          expr->CountBinOpFeedbackId());
   patch_site.EmitPatchInfo();
   __ bind(&done);
 
   // Store the value returned in rax.
@@ skipping 417 matching lines @@
   *context_length = 0;
   return previous_;
 }
 
 
 #undef __
 
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_X64