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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


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


================
Comment at: include/llvm/Analysis/ValueLattice.h:245
+    default:
+      return ConstantRange::makeSatisfyingICmpRegion(Pred, OtherCR)
+                .contains(CR);
----------------
sanjoy wrote:
> Why doesn't `ConstantRange::makeSatisfyingICmpRegion` work for `ICMP_NE` and `ICMP_EQ`?
> 
> Please also add a test case to unittests/ for this function.
`ConstantRange::makeSatisfyingICmpRegion` works on those cases too, thanks! I've added unit tests too 


https://reviews.llvm.org/D37591