[PATCH] D37591: [LVI] Move LVILatticeVal class to separate header file (NFC).

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/ValueLattice.h:27
+namespace llvm {
+class LVILatticeVal {
+  enum LVILatticeValueTy {
----------------
I think this should no longer be called LVILatticeVal, now that it is used outside LVI.


================
Comment at: include/llvm/Analysis/ValueLattice.h:84
+
+  bool isUndefined() const     { return Tag == undefined; }
+  bool isConstant() const      { return Tag == constant; }
----------------
Since you're moving the code anyway, can you also run clang-format over this file?


================
Comment at: include/llvm/Analysis/ValueLattice.h:179
+  /// one and returning true if anything changed.
+  bool mergeIn(const LVILatticeVal &RHS, const DataLayout &DL) {
+    if (RHS.isUndefined() || isOverdefined())
----------------
Now that this lives outside LVI, we need to be clearer about what this actually does (modifies *this to approximate both *this and RHS).


================
Comment at: include/llvm/Analysis/ValueLattice.h:233
+    // elements.
+    if (!isConstantRange() || !Other.isConstantRange())
+      return false;
----------------
This can also work for:

 - `ConstantExpr` for `ICMP_EQ`.
 - `undefined`



================
Comment at: include/llvm/Analysis/ValueLattice.h:245
+    default:
+      return ConstantRange::makeSatisfyingICmpRegion(Pred, OtherCR)
+                .contains(CR);
----------------
Why doesn't `ConstantRange::makeSatisfyingICmpRegion` work for `ICMP_NE` and `ICMP_EQ`?

Please also add a test case to unittests/ for this function.


https://reviews.llvm.org/D37591