[PATCH] D41903: [ValueLattice] Use union to shave off ptr size bytes from elements.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Bunch of minor comments, but once those addressed, likely good to go.



================
Comment at: include/llvm/Analysis/ValueLattice.h:58
+  union {
+    Constant *Const;
+    ConstantRange Range;
----------------
Minor: Calling this const is confusing.  I keep reading this as "Const... Range" which is misleading.  Val or ConstVal might be more clear.


================
Comment at: include/llvm/Analysis/ValueLattice.h:84
+  ValueLatticeElement(const ValueLatticeElement &Other) {
+    switch (Other.Tag) {
+    case constantrange:
----------------
You could initialize to undefined, then just invoke the operator= method on this.


================
Comment at: include/llvm/Analysis/ValueLattice.h:86
+    case constantrange:
+      new (&Range) ConstantRange(1, true);
+      Range = Other.Range;
----------------
Might make sense to add a ConstantRange copy constructor or move constructor.  You have this idiom in a couple of places.


================
Comment at: include/llvm/Analysis/ValueLattice.h:105
+    // destroy Range.
+    if (isConstantRange() && !Other.isConstantRange())
+      Range.~ConstantRange();
----------------
For error detection, it would be good to null out the Const field when switching away from something containing a Const.


================
Comment at: include/llvm/Analysis/ValueLattice.h:110
+    case constantrange:
+      if (!isConstantRange())
+        new (&Range) ConstantRange(1, true);
----------------
Another instance of the move constructor.


================
Comment at: include/llvm/Analysis/ValueLattice.h:186
+      Range.~ConstantRange();
     Tag = overdefined;
   }
----------------
Same point about nulling Const here.


================
Comment at: include/llvm/Analysis/ValueLattice.h:239
+      new (&Range) ConstantRange(1, true);
+      Range = NewR;
     }
----------------
You can use a move here.  Or a move constructor if there is one.


================
Comment at: include/llvm/Analysis/ValueLattice.h:280
     }
-    ConstantRange NewR = Range.unionWith(RHS.getConstantRange());
+    ConstantRange NewR = getConstantRange().unionWith(RHS.getConstantRange());
     if (NewR.isFullSet())
----------------
These uses of cover functions should be NFC on their own.  Please separate and commit to reduce the diff in next round of review.


https://reviews.llvm.org/D41903