Reduce copying, redundancy & repetition for codegen of comparison instructions

runtime/vm/assembler_mips.h

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #ifndef RUNTIME_VM_ASSEMBLER_MIPS_H_
 #define RUNTIME_VM_ASSEMBLER_MIPS_H_
 
 #ifndef RUNTIME_VM_ASSEMBLER_H_
 #error Do not include assembler_mips.h directly; use assembler.h instead.
 #endif
@@ skipping 159 matching lines @@
   static bool IsValidImm(int32_t value) {
     // We want both value and value + 1 to fit in an int16_t.
     return (-0x08000 <= value) && (value < 0x7fff);
   }
 
   void set_rel_op(RelationOperator value) {
     ASSERT(IsValidRelOp(value));
     bits_ = RelOpBits::update(value, bits_);
   }
 
+  void SetToInvalidState() {

[Vyacheslav Egorov (Google), 2017/06/15 11:32:46]

Maybe this method should be private?

[erikcorry, 2017/06/19 07:15:08]

Done.

+    bits_ = RelOpBits::update(INVALID_RELATION, bits_);
+  }
+
+  bool IsValid() { return rel_op() != INVALID_RELATION; }
+
   // Uninitialized condition.
   Condition() : ValueObject(), bits_(0) {}

[Vyacheslav Egorov (Google), 2017/06/15 11:32:46]

I would suggest that this one sets bits_ to invalid relation.

[erikcorry, 2017/06/19 07:15:09]

Done.

 
   // Copy constructor.
   Condition(const Condition& other) : ValueObject(), bits_(other.bits_) {}
 
   // Copy assignment operator.
   Condition& operator=(const Condition& other) {
     bits_ = other.bits_;
     return *this;
   }
 
   Condition(Register left,
             Register right,
             RelationOperator rel_op,
             int16_t imm = 0) {
     // At most one constant, ZR or immediate.
     ASSERT(!(((left == ZR) || (left == IMM)) &&
              ((right == ZR) || (right == IMM))));
     // Non-zero immediate value is only allowed for IMM.
     ASSERT((imm != 0) == ((left == IMM) || (right == IMM)));
     set_left(left);
     set_right(right);
-    set_rel_op(rel_op);
+    if (rel_op == INVALID_RELATION) {

[Vyacheslav Egorov (Google), 2017/06/15 11:32:46]

... and here ASSERT(rel_op != INVALID_RELATION)

[erikcorry, 2017/06/19 07:15:09]

I put the assert in the getters for the register numbers.

+      SetToInvalidState();
+    } else {
+      set_rel_op(rel_op);
+    }
     set_imm(imm);
   }
 
  private:
   static bool IsValidRelOp(RelationOperator value) {

[Vyacheslav Egorov (Google), 2017/06/15 11:32:46]

I would not be surprised if this gets completely optimized out - it is UB to
convert out-of-bounds values to enum. 

[erikcorry, 2017/06/19 07:15:09]

Ack

     return (AL <= value) && (value <= ULE);
   }
 
   static bool IsValidRegister(Register value) {
     return (ZR <= value) && (value <= IMM) && (value != AT);
   }
 
   void set_left(Register value) {
     ASSERT(IsValidRegister(value));
     bits_ = LeftBits::update(value, bits_);
@@ skipping 1474 matching lines @@
                                   Register value,
                                   Label* no_update);
 
   DISALLOW_ALLOCATION();
   DISALLOW_COPY_AND_ASSIGN(Assembler);
 };
 
 }  // namespace dart
 
 #endif  // RUNTIME_VM_ASSEMBLER_MIPS_H_