Suggestions for iDef infrastructure classes.

src/hydrogen-instructions.h

Index: src/hydrogen-instructions.h
diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index 3aa15e375a78aee6cbb4b31395cb1a87957ebe54..41d53394c0da1d838e1001507a421f673f66297b 100644
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -145,6 +145,7 @@ class LChunkBuilder;
   V(MathMinMax)                                \
   V(Mod)                                       \
   V(Mul)                                       \
+  V(NumericConstraint)                         \
   V(ObjectLiteral)                             \
   V(OsrEntry)                                  \
   V(OuterContext)                              \
@@ -836,6 +837,107 @@ class HValue: public ZoneObject {
   virtual void Verify() = 0;
 #endif
 
+  class NumericRelation {
+   public:
+    enum Kind { NONE, EQ, GT, GE, LT, LE, NE };
+    static const char* MnemonicFromKind(Kind kind) {
+      switch (kind) {
+        case NONE: return "NONE";
+        case EQ: return "EQ";
+        case GT: return "GT";
+        case GE: return "GE";
+        case LT: return "LT";
+        case LE: return "LE";
+        case NE: return "NE";
+        default:

[Jakob Kummerow, 2013/02/04 17:06:09]

default case not necessary (and not desirable)

+          UNREACHABLE();
+          return "UNREACHABLE";
+      }
+    }
+    const char* Mnemonic() const { return MnemonicFromKind((Kind) kind_); }

[Jakob Kummerow, 2013/02/04 17:06:09]

no C-style casts, please. Also, no casting necessary if kind_ has the right type
in the first place.

+
+    explicit NumericRelation(Kind kind) : kind_(kind) {}
+
+    static NumericRelation None() { return NumericRelation(NONE); }

[Jakob Kummerow, 2013/02/04 17:06:09]

Why do we need all of these when we have the generic constructor too?

+    static NumericRelation Eq() { return NumericRelation(EQ); }
+    static NumericRelation Gt() { return NumericRelation(GT); }
+    static NumericRelation Ge() { return NumericRelation(GE); }
+    static NumericRelation Lt() { return NumericRelation(LT); }
+    static NumericRelation Le() { return NumericRelation(LE); }
+    static NumericRelation Ne() { return NumericRelation(NE); }
+
+    bool operator=(const NumericRelation& other) {

[Jakob Kummerow, 2013/02/04 17:06:09]

I think you mean "operator==". But regardless, a .Equals() method is generally
preferred over operator overloading.

+      return kind_ == other.kind_;
+    }
+    bool operator!=(const NumericRelation& other) {
+      return kind_ != other.kind_;
+    }
+
+    // The semantics of "Reversed" is that if "x rel y" is true then also
+    // "y rel.Reversed x" is true.
+    NumericRelation Reversed() {
+      switch (kind_) {
+        case NONE: return None();
+        case EQ: return Ne();

[Jakob Kummerow, 2013/02/04 17:06:09]

No.

+        case GT: return Le();
+        case GE: return Lt();
+        case LT: return Ge();
+        case LE: return Gt();
+        case NE: return Eq();

[Jakob Kummerow, 2013/02/04 17:06:09]

No.

+        default:

[Jakob Kummerow, 2013/02/04 17:06:09]

no default case please

+          UNREACHABLE();
+          return None();
+      }
+    }
+
+    // The semantics of "Implies" is that if "x rel y" is true then also
+    // "(x + delta) other (y + offset)" is true.
+    bool Implies(NumericRelation other, int32_t offset = 0, int delta = 0) {

[Jakob Kummerow, 2013/02/04 17:06:09]

Why do |offset| and |delta| have different types?
Also, they should have clearer names, maybe x_delta and y_delta.

+      switch (kind_) {
+        case NONE: return false;
+        case EQ: return (other.kind_ == EQ && offset == delta) ||
+            (other.kind_ == EQ && offset == delta) ||

[Jakob Kummerow, 2013/02/04 17:06:09]

duplicate condition

+            (other.kind_ == GT && offset > delta) ||

[Jakob Kummerow, 2013/02/04 17:06:09]

No. And many of the conditions below are mixed up as well.

+            (other.kind_ == GE && offset <= delta) ||
+            (other.kind_ == LT && offset < delta) ||
+            (other.kind_ == LE && offset <= delta) ||
+            (other.kind_ == NE && offset != delta);
+        case GT: return (other.kind_ == GT && offset >= delta) ||
+            (other.kind_ == GE && offset > delta);
+        case GE: return (other.kind_ == GE && offset >= delta) ||
+            (other.kind_ == GT && offset >= delta + 1);

[Jakob Kummerow, 2013/02/04 17:06:09]

Why the sudden change from "a > b" to "a >= b+1"?

+        case LT: return (other.kind_ == LT && offset <= delta) ||
+            (other.kind_ == LE && offset < delta);
+        case LE: return (other.kind_ == LE && offset <= delta) ||
+            (other.kind_ == LT && offset <= delta - 1);
+        case NE: return (other.kind_ == NE && offset == delta);
+        default:

[Jakob Kummerow, 2013/02/04 17:06:09]

no default case please

+          UNREACHABLE();
+          return false;
+      }
+    }
+
+   private:
+    int8_t kind_;

[Jakob Kummerow, 2013/02/04 17:06:09]

Why not "Kind kind_;"?

+  };
+
+  bool IsInteger32ConstantValue();

[Jakob Kummerow, 2013/02/04 17:06:09]

Where are these implemented?

+  int32_t GetInteger32ConstantValue();
+  bool DecomposeValuePlusConstant(HValue** value, int32_t* constant);
+
+  bool IsRelationTrue(NumericRelation relation,
+                      HValue* other,
+                      int32_t offset = 0) {
+    bool result = CheckRelation(relation, other, offset) ||

[Jakob Kummerow, 2013/02/04 17:06:09]

"this->CheckRelation" won't compile, because |this| is HValue and CheckRelation
is defined on HNumericConstraint.

+        other->CheckRelation(relation.Reversed(), this, -offset);
+    return result;
+  }
+
+  HValue* InsertNumericConstraint(HInstruction* insertion_point,
+                                  NumericRelation relation,
+                                  HValue* other,
+                                  int32_t offset = 0);
+
  protected:
   // This function must be overridden for instructions with flag kUseGVN, to
   // compare the non-Operand parts of the instruction.
@@ -1122,6 +1224,57 @@ class HDummyUse: public HTemplateInstruction<1> {
 };
 
 
+class HNumericConstraint : public HTemplateInstruction<2> {
+ public:
+  static void AddImpliedConstraints(HInstruction* insertion_point,

[Jakob Kummerow, 2013/02/04 17:06:09]

I don't see an implementation for this.

+                                    HValue* input,

[Jakob Kummerow, 2013/02/04 17:06:09]

I'd like a more descriptive name here. "constrained_value"?

+                                    NumericRelation constraint,

[Jakob Kummerow, 2013/02/04 17:06:09]

Since we have classes calld [H]NumericConstraint and NumericRelation, please
don't call a Relation constraint. (Again below.)

+                                    HValue* value,

[Jakob Kummerow, 2013/02/04 17:06:09]

"value" is a bit too generic. How about "reference"? Or "reference_point"? Or
"relative_to"? (Also applies to occurrences below.)

+                                    int delta = 0);
+
+  static HNumericConstraint* New(HInstruction* insertion_point,

[Jakob Kummerow, 2013/02/04 17:06:09]

So, we have five method with almost equal signatures and related behavior:
HValue::InsertNumericConstraint(), HNumericConstraint::New(),
HNumericConstraints::Create(), HNumericConstraint::AddImpliedConstraints(), and
the ctor HNumericConstraints(). Why? What's the difference? How am I supposed to
know which to use when? Can we unify them down to one or two versions?

+                                 HValue* input,
+                                 NumericRelation constraint,
+                                 HValue* value,
+                                 int delta = 0);
+
+  HValue* input() { return OperandAt(0); }
+  HValue* value() { return OperandAt(1); }
+  NumericRelation constraint() { return constraint_; }
+  int delta() { return delta_; }
+
+  virtual int RedefinedOperandIndex() { return 0; }
+
+  virtual Representation RequiredInputRepresentation(int index) {
+    return input()->RequiredInputRepresentation(index);
+  }
+
+  virtual void PrintDataTo(StringStream* stream);
+  //virtual void AddInformativeDefinitions();

[Jakob Kummerow, 2013/02/04 17:06:09]

?

+  virtual bool CheckRelation(NumericRelation relation,
+                             HValue* other,
+                             int32_t offset);
+
+
+  DECLARE_CONCRETE_INSTRUCTION(NumericConstraint)
+
+ private:
+  explicit HNumericConstraint(HInstruction* insertion_point,
+                              HValue* input,
+                              NumericRelation constraint,
+                              HValue* value,
+                              int delta);
+
+  static HNumericConstraint* Create(HInstruction* insertion_point,
+                                    HValue* input,
+                                    NumericRelation constraint,
+                                    HValue* value,
+                                    int delta = 0);
+  NumericRelation constraint_;
+  int delta_;
+};
+
+
 // We insert soft-deoptimize when we hit code with unknown typefeedback,
 // so that we get a chance of re-optimizing with useful typefeedback.
 // HSoftDeoptimize does not end a basic block as opposed to HDeoptimize.