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

src/x64/macro-assembler-x64.h

Index: src/x64/macro-assembler-x64.h
diff --git a/src/x64/macro-assembler-x64.h b/src/x64/macro-assembler-x64.h
index ca13ca3955423c54f3bcc5136186b4adc9ba3600..d57b4da4126f1f34bd7f451209159aabe7f21c8e 100644
--- a/src/x64/macro-assembler-x64.h
+++ b/src/x64/macro-assembler-x64.h
@@ -375,12 +375,60 @@ class MacroAssembler: public Assembler {
   // ---------------------------------------------------------------------------
   // 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.
@@ -484,10 +532,17 @@ class MacroAssembler: public Assembler {
   // 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,
@@ -549,8 +604,7 @@ class MacroAssembler: public Assembler {
   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
@@ -558,12 +612,11 @@ class MacroAssembler: public Assembler {
   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!
@@ -573,31 +626,23 @@ class MacroAssembler: public Assembler {
               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,
@@ -606,8 +651,7 @@ class MacroAssembler: public Assembler {
   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,
@@ -650,7 +694,8 @@ class MacroAssembler: public Assembler {
 
   void SmiShiftLeftConstant(Register dst,
                             Register src,
-                            int shift_value);
+                            int shift_value,
+                            const SmiInstructionWrapper& wrapper);
   void SmiShiftLogicalRightConstant(Register dst,
                                   Register src,
                                   int shift_value,
@@ -664,7 +709,8 @@ class MacroAssembler: public Assembler {
   // 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.
@@ -722,6 +768,9 @@ class MacroAssembler: public Assembler {
   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.
@@ -774,11 +823,6 @@ class MacroAssembler: public Assembler {
   // 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);