Optimizes an assembly-language conversion from int to Smi in codegen.

src/codegen-ia32.cc

 // Copyright 2006-2008 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 1108 matching lines @@
       } else {
         int shift_value = int_value & 0x1f;  // only least significant 5 bits
         DeferredCode* deferred =
         new DeferredInlinedSmiOperation(this, Token::SHL, shift_value,
                                         overwrite_mode);
         frame_->Pop(eax);
         __ test(eax, Immediate(kSmiTagMask));
         __ mov(ebx, Operand(eax));
         __ j(not_zero, deferred->enter(), not_taken);
         __ sar(ebx, kSmiTagSize);
         __ shl(ebx, shift_value);

[Kevin Millikin (Chromium), 2009/01/23 18:48:08]

It's late in the day so I could be wrong.

Do we really need to shift right kSmiTagSize, shift left shift_value, and then
shift left kSmiTagSize to reapply the tag?  Analogous to a multiply, can't we
just shift the tagged value if the tag is zero?

-        // This is the Smi check for the shifted result.
-        // After signed subtraction of 0xc0000000, the valid 
-        // Smis are positive.
-        __ cmp(ebx, 0xc0000000);
-        __ j(sign, deferred->enter(), not_taken); 
-        // tag result and store it in TOS (eax)
-        ASSERT(kSmiTagSize == times_2);  // adjust code if not the case
-        __ lea(eax, Operand(ebx, ebx, times_1, kSmiTag));
+        // Convert the int to a Smi, and check that it is in
+        // the range of valid Smis.
+        ASSERT(kSmiTagSize == times_2);  // Adjust code if not true.

[iposva, 2009/01/23 16:58:38]

kSmiTagSize is a number of bits, times_2 is a instruction operand encoding
field. This assert is comparing apples and pinecones. I will fix this and I also
realize that there are many other places of this misuse.

+        ASSERT(kSmiTag == 0);  // Adjust code if not true.
+        __ add(ebx, Operand(ebx));
+        __ j(overflow, deferred->enter(), not_taken);
+        __ mov(eax, Operand(ebx));
+
         __ bind(deferred->exit());
         frame_->Push(eax);
       }
       break;
     }
 
     case Token::BIT_OR:
     case Token::BIT_XOR:
     case Token::BIT_AND: {
       DeferredCode* deferred = NULL;
@@ skipping 4162 matching lines @@
 
   // Slow-case: Go through the JavaScript implementation.
   __ bind(&slow);
   __ InvokeBuiltin(Builtins::INSTANCE_OF, JUMP_FUNCTION);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal