[ValueLattice] Add move constructor

[nikic]

Summary

Following the rule of five, declare move constructor and move assignment operator for ValueLatticeElement. This allows moving the ConstantRange (and the APInts it contains) rather than copying it.

This gives no improvement for most practical purposes, because we tend to work on ranges with <= 64 bit, in which case copies are cheap. This would only make a difference if the APInts have allocated backing storage.

So, not sure if worthwhile, but I figured it put it out there anyway...

[nikic]

include/llvm/Analysis/ValueLattice.h

129 ↗

(On Diff #258517)

Happy to commit this part separately: This nulling of ConstVal looks unnecessary to me. ConstVal is not an owned pointer, so this doesn't free it. If we change to a state that does not the ConstVal, then the value simply does not matter and we can leave it alone.

153 ↗

(On Diff #258517)

I went for a slightly simpler implementation than the copy assignment operator here by always destroying and constructing, rather than trying to do an assignment if we go from one range to another range. That makes (theoretical, but probably not practical) sense for copy assignment, but not for move assignment.

[fhahn]

include/llvm/Analysis/ValueLattice.h

129 ↗

(On Diff #258517)

It would be great if you could commit that separately.

153 ↗

(On Diff #258517)

Yeah that should be fine. The main reason was to avoid constructing a range unnecessarily, but as the vast majority of ranges has tbitwidths <= 64 that should not matter in practice. Best to keep the assignment & move operators in sync in that respect.

[nikic]

Rebase, simplify copy constructor.

[nikic]

Base assignment operators on constructors rather than the other way around.

[nikic]

include/llvm/Analysis/ValueLattice.h

153 ↗

(On Diff #258517)

After doing that change, I think it makes sense to implement the assignment operators in terms of destruct + construct, rather than implementing the constructors in terms of default construct and assign. The copy/move constructors are the more primitive operations.

I wasn't completely sure whether writing new (this) is safe, but it looks like there's already parts of LLVM using exactly this pattern, such as ImplicitConversionSequence.

[nikic]

include/llvm/Analysis/ValueLattice.h

145 ↗

(On Diff #258594)

A better implementation of this would be:

memcpy(this, &Other, sizeof(ValueLatticeElement));
Other.Tag = unknown;

I'm not sure though whether this is legal C++ or not :)

[fhahn]

LGTM, thanks. I think it would be good to update the description after the latest changes.

include/llvm/Analysis/ValueLattice.h

107 ↗

(On Diff #258594)

might be a good opportunity to update Const -> ConstVal.

llvm/include/llvm/Analysis/ValueLattice.h

class ValueLatticeElement {
   /// The union either stores a pointer to a constant or a constant range,
   /// associated to the lattice element. We have to ensure that Range is
   /// initialized or destroyed when changing state to or from constantrange.
   union {
     Constant *ConstVal;
     ConstantRange Range;
   };
 
+  /// Destroy contents of lattice value, without destructing the object.
+  void destroy() {
+    switch (Tag) {
+    case overdefined:
+    case unknown:
+    case undef:
+    case constant:
+    case notconstant:
+      break;
+    case constantrange_including_undef:
+    case constantrange:
+      Range.~ConstantRange();
+      break;
+    };
+  }
+
 public:
   /// Struct to control some aspects related to merging constant ranges.
   struct MergeOptions {
     /// The merge value may include undef.
     bool MayIncludeUndef;
 
     /// Handle repeatedly extending a range by going to overdefined after a
     /// number of steps.
struct MergeOptions {
     }
 
     MergeOptions &setCheckWiden(bool V = true) {
       CheckWiden = V;
       return *this;
     }
   };
 
-  // Const and Range are initialized on-demand.
+  // ConstVal and Range are initialized on-demand.
   ValueLatticeElement() : Tag(unknown) {}
 
-  /// Custom destructor to ensure Range is properly destroyed, when the object
-  /// is deallocated.
-  ~ValueLatticeElement() {
-    switch (Tag) {
-    case overdefined:
-    case unknown:
-    case undef:
+  ~ValueLatticeElement() { destroy(); }
+
+  ValueLatticeElement(const ValueLatticeElement &Other) : Tag(Other.Tag) {
+    switch (Other.Tag) {
+    case constantrange:
+    case constantrange_including_undef:
+      new (&Range) ConstantRange(Other.Range);
+      NumRangeExtensions = Other.NumRangeExtensions;
+      break;
     case constant:
     case notconstant:
+      ConstVal = Other.ConstVal;
       break;
-    case constantrange_including_undef:
-    case constantrange:
-      Range.~ConstantRange();
+    case overdefined:
+    case unknown:
+    case undef:
       break;
-    };
     }
-
-  /// Custom copy constructor, to ensure Range gets initialized when
-  /// copying a constant range lattice element.
-  ValueLatticeElement(const ValueLatticeElement &Other) : Tag(unknown) {
-    *this = Other;
   }
 
-  /// Custom assignment operator, to ensure Range gets initialized when
-  /// assigning a constant range lattice element.
-  ValueLatticeElement &operator=(const ValueLatticeElement &Other) {
-    // If we change the state of this from constant range to non constant range,
-    // destroy Range.
-    if (isConstantRange() && !Other.isConstantRange())
-      Range.~ConstantRange();
-
+  ValueLatticeElement(ValueLatticeElement &&Other) : Tag(Other.Tag) {
     switch (Other.Tag) {
     case constantrange:
     case constantrange_including_undef:
-      if (!isConstantRange())
-        new (&Range) ConstantRange(Other.Range);
-      else
-        Range = Other.Range;
+      new (&Range) ConstantRange(std::move(Other.Range));
       NumRangeExtensions = Other.NumRangeExtensions;
       break;
     case constant:
     case notconstant:
       ConstVal = Other.ConstVal;
       break;
     case overdefined:
     case unknown:
     case undef:
       break;
     }
-    Tag = Other.Tag;
+    Other.Tag = unknown;
+  }
+
+  ValueLatticeElement &operator=(const ValueLatticeElement &Other) {
+    destroy();
+    new (this) ValueLatticeElement(Other);
+    return *this;
+  }
+
+  ValueLatticeElement &operator=(ValueLatticeElement &&Other) {
+    destroy();
+    new (this) ValueLatticeElement(std::move(Other));
     return *this;
   }
 
   static ValueLatticeElement get(Constant *C) {
     ValueLatticeElement Res;
     if (isa<UndefValue>(C))
       Res.markUndef();
     else
if (isConstant() && isa<ConstantInt>(getConstant())) {
       return *getConstantRange().getSingleElement();
     }
     return None;
   }
 
   bool markOverdefined() {
     if (isOverdefined())
       return false;
-    if (isConstantRange())
-      Range.~ConstantRange();
+    destroy();
     Tag = overdefined;
     return true;
   }
 
   bool markUndef() {
     if (isUndef())
       return false;