[PATCH] D76611: [SCCP] Use ranges for predicate info conditions.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

Thank you very much Eli!



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1299
+      LatticeVal &IV = ValueState[I];
+      if (CopyOf->getType()->isIntegerTy()) {
+        auto NewCR =
----------------
efriedma wrote:
> efriedma wrote:
> > fhahn wrote:
> > > efriedma wrote:
> > > > Do we want to try to handle the case where we have a constant expression of integer type?
> > > Ah yes, I've change it to use ranges only if either operand is a constant range.
> > > 
> > > I've added a test with a compare of 2 constant ranges. Currently this just swaps the known values and is not too helpful in that case (f14_constexpr2). 
> > > 
> > > Also, would it be legal to fold something like `i1 icmp eq (i32 ptrtoint (i32* @B to i32), i32 ptrtoint (i32* @B to i32)` to `true`? If it is, it looks like ConstantExpr currently misses that fold.
> > I think the order of the conditions might be backwards? Consider if OriginalVal is a ConstantRange, and CondVal is a Constant.
> > 
> > -----
> > 
> > I briefly tried the following, and it seems to fold (just passing it to "opt -S"):
> > 
> > @B = global i32 0
> > @C = global i1 icmp eq (i32 ptrtoint (i32* @B to i32), i32 ptrtoint (i32* @B to i32))
> Err, actually, hmm... in general, there's sort a conflict here between Constant and ConstantRange here; in more complicated cases, you could end up accidentally landing on overdefined by merging a Constant and a ConstantRange.
> 
> I guess it's unlikely to matter very much in practice; constant expressions are relatively rare.
Yes I think potentially are cases involving constant ranges where we might go to overdefined using ranges unnecessarily. Maybe it would be best to try both and chose the better result, although it sometimes is not clear if a larger constant range or a constant Expr is better for further analysis. I think it would be interesting to follow up on that in future patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76611