[PATCH] D60582: [IPSCCP] Use ValueLatticeElement instead of LatticeVal (NFCI)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'm skeptical of the way this stack is handling forcedconstant... but probably makes more sense to discuss on D61314 <https://reviews.llvm.org/D61314>.



================
Comment at: llvm/include/llvm/Analysis/ValueLattice.h:250
+      if (getConstantRange().contains(NewR))
+        return false;
+
----------------
This is just a drive-by fix?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:345-346
+  ConstantInt *getConstantInt(const LatticeVal &IV) const {
+    if (IV.isConstant())
+      return IV.getConstantInt();
+    return cast_or_null<ConstantInt>(getConstant(IV));
----------------
Is the IV.isConstant() here actually useful?  Don't we always resolve constant integers to a ConstantRange?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:408
+    // loop. If IV is a constant range, it means we already set it once. If
+    // MergeWithV would extend IV, mark V as overdefined.
+    if (Widen && IV.isConstantRange() && MergeWithV.isConstantRange() &&
----------------
This probably could be refined to handle cases that aren't actually loops, but probably fine as a first shot.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:719
+  if (!isUnknownOrConstant(getValueState(&PN))) {
+    return (void)markOverdefined(&PN);
+  }
----------------
The point of "!isUnknownOrConstant()" instead of isOverdefined() is to exclude constant ranges?  If that's right, could you use a positive name like "isOverdefinedOrConstantRange"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60582/new/

https://reviews.llvm.org/D60582