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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

ping



================
Comment at: llvm/include/llvm/Analysis/ValueLattice.h:250
+      if (getConstantRange().contains(NewR))
+        return false;
+
----------------
efriedma wrote:
> This is just a drive-by fix?
Yes, I moved that out to D73240


================
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));
----------------
efriedma wrote:
> Is the IV.isConstant() here actually useful?  Don't we always resolve constant integers to a ConstantRange?
> Is the IV.isConstant() here actually useful? 

Not really, the check is not required, as getConstant() already handles that already.

> Don't we always resolve constant integers to a ConstantRange?

I *think* I encountered some cases with integer constant expressions that do not resolve to a concrete integer (e.g. involving `ptrtoint`).


================
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() &&
----------------
efriedma wrote:
> This probably could be refined to handle cases that aren't actually loops, but probably fine as a first shot.
Agreed, I am planning on improving that as another follow-up, once the series of changes in in, so we have a better chance to spot issues through testing.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:719
+  if (!isUnknownOrConstant(getValueState(&PN))) {
+    return (void)markOverdefined(&PN);
+  }
----------------
efriedma wrote:
> The point of "!isUnknownOrConstant()" instead of isOverdefined() is to exclude constant ranges?  If that's right, could you use a positive name like "isOverdefinedOrConstantRange"?
It excludes constant ranges with more than a single element and should cover exactly the same cases as the original LatticeVal::isOverdefined. I've changed the name to isOverdefined and also updated the comments for the helper functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60582