Optionally use 31-bits SMI value for 64-bit system

src/x64/macro-assembler-x64.h

 // 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 357 matching lines @@
   // Store the function for the given builtin in the target register.
   void GetBuiltinFunction(Register target, Builtins::JavaScript id);
 
   // Store the code object for the given builtin in the target register.
   void GetBuiltinEntry(Register target, Builtins::JavaScript id);
 
 
   // ---------------------------------------------------------------------------
   // Smi tagging, untagging and operations on tagged smis.
 
+  class SmiInstructionWrapper {

[danno, 2013/08/19 21:47:44]

My general feedback to this class is that it's too complex. You seem to be
mixing responsibilities. e.g. whether a particular SMI operation needs to check
for overflow is a decision of the calling site of the macro instruction, less so
a policy of whether you're generating full code gen code or not. Same for
NeedsCheckMinusZero, it seems like something that is passed in separately. 

How about this instead (including a rename thatI think is important):

class SmiOperationOverflowHandler {
 public:
  virtual void BailoutIf(Condition cc) const = 0;
}

// Use this for callers that don't care about overflow at all.
class IgnoreSmiOverflowHandler : public SmiOperationOverflowHandler{
  virtual void BailoutIf(Condition cc) const {}
}

class JumpOnSmiOverflowHandler :  public SmiOperationOverflowHandler {
  virtual void BailoutIf(Condition cc) const { DeoptimizeIf(cc, env) }
}

class DeoptOnSmiOverflowHandler : public SmiOperationOverflowHandler {
  SmiOperationOverflowHandler(bool can_overflow) {...}
  virtual void BailoutIf(Condition cc) const { if (can_overflow_) {
DeoptimizeIf(cc, env); } }
}

NeedsKeepSourceOperandsIntact also doesn't seem to belong here. It seems that
there are two different types of Smi operations. Some, like Add and Sub, have
some callers that want to make sure that their input register are preserved in
certain situations... this is a per-call-site decision. Others, like
SmiShiftLeft, didn't care if dst is the same as src1/src2, clobbering was OK.
That should still remain the case when you add 31-bit smi support, which might
mean that you need to pass in an addition temp register so that you don't have
to write different or convoluted code at the caller site in the 31/32 bit smi
cases.

+   public:
+    SmiInstructionWrapper() { }
+    virtual ~SmiInstructionWrapper() { }
+    virtual bool NeedsCheckOverflow() const = 0;
+    virtual bool NeedsCheckMinusZero() const = 0;

[danno, 2013/08/19 21:47:44]

You never call NeedsCheckMinusZero anywhere. Can you remove it.

[haitao.feng, 2013/08/20 15:09:30]

I have planned to use it for SmiMul. If the caller is in lithium, we could avoid
check for minus zero if HValue::kBailoutOnMinusZero flag is false.

[danno, 2013/08/20 15:58:14]

Yes, but this is specific to the Mul operation, seems like it could just be a
"MinusZeroHandlingMode" parameter to the SmiMul/SmiDiv macro assembler
instruction?

[haitao.feng, 2013/08/21 13:18:03]

We will handle this when https://codereview.chromium.org/22600005/ is landed.
Currently the difference between Lithium and FullCodeGen/Stub when calling SMI
macro instruction is:
  1) dst and src1 are same from Lithium, and we do not need to keep src1 intact.
  2) overflow check could be omitted.
  3) minusZero check could be omitted. 

I will think about your recommendations to abstract those difference.

+    virtual bool NeedsKeepSourceOperandsIntact() const = 0;

[danno, 2013/08/19 21:47:44]

I think you can avoid this one, too. See my comments in the .cc file.

+    virtual void BailoutIf(Condition cc) const = 0;
+  };
+
+  class SafeSmiInstructionWrapper : public SmiInstructionWrapper {

[danno, 2013/08/19 21:47:44]

This class is never used, please remove it.

[haitao.feng, 2013/08/20 15:09:30]

I have planned to unify the interface. So the current SMI functions which do not
check overflow/minusZero could use a singleton SafeSmiInstrucitonWrapper.

+   public:
+    SafeSmiInstructionWrapper() { }
+    virtual ~SafeSmiInstructionWrapper() { }
+    virtual bool NeedsCheckOverflow() const { return false; }
+    virtual bool NeedsCheckMinusZero() const { return false; }
+    virtual bool NeedsKeepSourceOperandsIntact() const { return false; }
+    virtual void BailoutIf(Condition cc) const { }
+  };
+
+  class StrictSmiInstructionWrapper : public SmiInstructionWrapper {
+   public:
+    StrictSmiInstructionWrapper(MacroAssembler* macro_assembler,
+                                Label* on_invalid,
+                                Label::Distance near_jump = Label::kFar)
+        : macro_assembler_(macro_assembler),
+          on_invalid_(on_invalid),
+          near_jump_(near_jump) { }
+    virtual ~StrictSmiInstructionWrapper() { }
+    virtual bool NeedsCheckOverflow() const { return true; }
+    virtual bool NeedsCheckMinusZero() const { return true; }
+    virtual bool NeedsKeepSourceOperandsIntact() const { return true; }
+    virtual void BailoutIf(Condition cc) const {
+      ASSERT_NOT_NULL(on_invalid_);
+      macro_assembler_->j(cc, on_invalid_, near_jump_);
+    }
+
+   private:
+    MacroAssembler* macro_assembler_;
+    Label* on_invalid_;
+    Label::Distance near_jump_;
+  };
+
   void InitializeSmiConstantRegister() {
     movq(kSmiConstantRegister,
          reinterpret_cast<uint64_t>(Smi::FromInt(kSmiConstantRegisterValue)),
          RelocInfo::NONE64);
   }
 
+  // Support for constant splitting.
+  bool IsUnsafeInt(const int x);

[danno, 2013/08/19 21:47:44]

shouldn't this be a int32_t just to be clear?

[haitao.feng, 2013/08/20 15:09:30]

I will change this when I gather all the SMI functions together.

+  void SafeMove(Register dst, Smi* src);
+  void SafePush(Smi* src);
+
   // Conversions between tagged smi values and non-tagged integer values.
 
   // Tag an integer value. The result must be known to be a valid smi value.
   // Only uses the low 32 bits of the src register. Sets the N and Z flags
   // based on the value of the resulting smi.
   void Integer32ToSmi(Register dst, Register src);
 
   // Stores an integer32 value into a memory field that already holds a smi.
   void Integer32ToSmiField(const Operand& dst, Register src);
 
@@ skipping 83 matching lines @@
   // and to one if it isn't.
   void CheckSmiToIndicator(Register dst, Register src);
   void CheckSmiToIndicator(Register dst, const Operand& src);
 
   // Test-and-jump functions. Typically combines a check function
   // above with a conditional jump.
 
   // Jump if the value cannot be represented by a smi.
   void JumpIfNotValidSmiValue(Register src, Label* on_invalid,
                               Label::Distance near_jump = Label::kFar);
+  // Jump if the value can be represented by a smi.
+  void JumpIfValidSmiValue(Register src, Label* on_valid,
+                           Label::Distance near_jump = Label::kFar);
 
   // Jump if the unsigned integer value cannot be represented by a smi.
   void JumpIfUIntNotValidSmiValue(Register src, Label* on_invalid,
                                   Label::Distance near_jump = Label::kFar);
+  // Jump if the unsigned integer value can be represented by a smi.
+  void JumpIfUIntValidSmiValue(Register src, Label* on_valid,
+                               Label::Distance near_jump = Label::kFar);
+
 
   // Jump to label if the value is a tagged smi.
   void JumpIfSmi(Register src,
                  Label* on_smi,
                  Label::Distance near_jump = Label::kFar);
 
   // Jump to label if the value is not a tagged smi.
   void JumpIfNotSmi(Register src,
                     Label* on_not_smi,
                     Label::Distance near_jump = Label::kFar);
@@ skipping 41 matching lines @@
 
   // Add an integer constant to a tagged smi, giving a tagged smi as result.
   // No overflow testing on the result is done.
   void SmiAddConstant(const Operand& dst, Smi* constant);
 
   // Add an integer constant to a tagged smi, giving a tagged smi as result,
   // or jumping to a label if the result cannot be represented by a smi.
   void SmiAddConstant(Register dst,
                       Register src,
                       Smi* constant,
-                      Label* on_not_smi_result,
-                      Label::Distance near_jump = Label::kFar);
+                      const SmiInstructionWrapper& wrapper);
 
   // Subtract an integer constant from a tagged smi, giving a tagged smi as
   // result. No testing on the result is done. Sets the N and Z flags
   // based on the value of the resulting integer.
   void SmiSubConstant(Register dst, Register src, Smi* constant);
 
   // Subtract an integer constant from a tagged smi, giving a tagged smi as
-  // result, or jumping to a label if the result cannot be represented by a smi.
+  // result, or bailout if the result cannot be represented by a smi.
   void SmiSubConstant(Register dst,
                       Register src,
                       Smi* constant,
-                      Label* on_not_smi_result,
-                      Label::Distance near_jump = Label::kFar);
+                      const SmiInstructionWrapper& wrapper);
 
   // Negating a smi can give a negative zero or too large positive value.
   // NOTICE: This operation jumps on success, not failure!
   void SmiNeg(Register dst,
               Register src,
               Label* on_smi_result,
               Label::Distance near_jump = Label::kFar);
 
   // Adds smi values and return the result as a smi.
-  // If dst is src1, then src1 will be destroyed, even if
-  // the operation is unsuccessful.
   void SmiAdd(Register dst,
               Register src1,
               Register src2,
-              Label* on_not_smi_result,
-              Label::Distance near_jump = Label::kFar);
+              const SmiInstructionWrapper& wrapper);
   void SmiAdd(Register dst,
               Register src1,
               const Operand& src2,
-              Label* on_not_smi_result,
-              Label::Distance near_jump = Label::kFar);
-
+              const SmiInstructionWrapper& wrapper);
   void SmiAdd(Register dst,
               Register src1,
               Register src2);
 
   // Subtracts smi values and return the result as a smi.
-  // If dst is src1, then src1 will be destroyed, even if
-  // the operation is unsuccessful.
   void SmiSub(Register dst,
               Register src1,
               Register src2,
-              Label* on_not_smi_result,
-              Label::Distance near_jump = Label::kFar);
+              const SmiInstructionWrapper& wrapper);
 
   void SmiSub(Register dst,
               Register src1,
               Register src2);
 
   void SmiSub(Register dst,
               Register src1,
               const Operand& src2,
-              Label* on_not_smi_result,
-              Label::Distance near_jump = Label::kFar);
+              const SmiInstructionWrapper& wrapper);
 
   void SmiSub(Register dst,
               Register src1,
               const Operand& src2);
 
   // Multiplies smi values and return the result as a smi,
   // if possible.
   // If dst is src1, then src1 will be destroyed, even if
   // the operation is unsuccessful.
   void SmiMul(Register dst,
@@ skipping 22 matching lines @@
   void SmiNot(Register dst, Register src);
   void SmiAnd(Register dst, Register src1, Register src2);
   void SmiOr(Register dst, Register src1, Register src2);
   void SmiXor(Register dst, Register src1, Register src2);
   void SmiAndConstant(Register dst, Register src1, Smi* constant);
   void SmiOrConstant(Register dst, Register src1, Smi* constant);
   void SmiXorConstant(Register dst, Register src1, Smi* constant);
 
   void SmiShiftLeftConstant(Register dst,
                             Register src,
-                            int shift_value);
+                            int shift_value,
+                            const SmiInstructionWrapper& wrapper);
   void SmiShiftLogicalRightConstant(Register dst,
                                   Register src,
                                   int shift_value,
                                   Label* on_not_smi_result,
                                   Label::Distance near_jump = Label::kFar);
   void SmiShiftArithmeticRightConstant(Register dst,
                                        Register src,
                                        int shift_value);
 
   // Shifts a smi value to the left, and returns the result if that is a smi.
   // Uses and clobbers rcx, so dst may not be rcx.
   void SmiShiftLeft(Register dst,
                     Register src1,
-                    Register src2);
+                    Register src2,
+                    Label* on_not_smi_result);
   // Shifts a smi value to the right, shifting in zero bits at the top, and
   // returns the unsigned intepretation of the result if that is a smi.
   // Uses and clobbers rcx, so dst may not be rcx.
   void SmiShiftLogicalRight(Register dst,
                             Register src1,
                             Register src2,
                             Label* on_not_smi_result,
                             Label::Distance near_jump = Label::kFar);
   // Shifts a smi value to the right, sign extending the top, and
   // returns the signed intepretation of the result. That will always
@@ skipping 37 matching lines @@
   }
 
   void Move(const Operand& dst, Smi* source) {
     Register constant = GetSmiConstant(source);
     movq(dst, constant);
   }
 
   void Push(Smi* smi);
   void Test(const Operand& dst, Smi* source);
 
+  void PushInt64AsTwoSmis(Register src, Register scratch = kScratchRegister);
+  void PopInt64AsTwoSmis(Register dst, Register scratch = kScratchRegister);
+  static bool IsUnsafeSmiOperator(Token::Value op);
 
   // ---------------------------------------------------------------------------
   // String macros.
 
   // If object is a string, its map is loaded into object_map.
   void JumpIfNotString(Register object,
                        Register object_map,
                        Label* not_string,
                        Label::Distance near_jump = Label::kFar);
 
@@ skipping 32 matching lines @@
   // ---------------------------------------------------------------------------
   // Macro instructions.
 
   // Load a register with a long value as efficiently as possible.
   void Set(Register dst, int64_t x);
   void Set(const Operand& dst, int64_t x);
 
   // Move if the registers are not identical.
   void Move(Register target, Register source);
 
-  // Support for constant splitting.
-  bool IsUnsafeInt(const int x);
-  void SafeMove(Register dst, Smi* src);
-  void SafePush(Smi* src);
-
   // Bit-field support.
   void TestBit(const Operand& dst, int bit_index);
 
   // Handle support
   void Move(Register dst, Handle<Object> source);
   void Move(const Operand& dst, Handle<Object> source);
   void Cmp(Register dst, Handle<Object> source);
   void Cmp(const Operand& dst, Handle<Object> source);
   void Cmp(Register dst, Smi* src);
   void Cmp(const Operand& dst, Smi* src);
@@ skipping 749 matching lines @@
     masm->popfq();                                                           \
   }                                                                          \
   masm->
 #else
 #define ACCESS_MASM(masm) masm->
 #endif
 
 } }  // namespace v8::internal
 
 #endif  // V8_X64_MACRO_ASSEMBLER_X64_H_