[PATCH] D78667: [ValueLattice] Merging unknown with empty CR is unknown.

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/include/llvm/Analysis/ValueLattice.h:359
+        return false;
       return markOverdefined();
+    }
----------------
nikic wrote:
> fhahn wrote:
> > nikic wrote:
> > > Can't we do this for the `isUndef()` case as well, i.e. return false here unconditionally?
> > > Can't we do this for the isUndef() case as well, i.e. return false here unconditionally?
> > 
> > I can't think of a reason now why not, but it seems like this has been there for a long time. Not sure if that is/was a LVI specific assumption in the original code. I've adjusted the code and now empty ranges should never reach markConstantRange.
> > 
> > (@efriedma I'll include the responses to your comments inline here, as they seem to fit with the context here)
> > 
> > > markConstantRange with an empty range should be equivalent to markUnknown (i.e. it shouldn't do anything at all).
> > 
> > Eli, do you by any chance remember why the restriction was there in the first place? I agree, it seems like we should just treat empty set as if merging with unknown. I think the better fix would be to not create constantrange lattice values in the first place with empty ranges (needs a fix in getRange()).
> > 
> > > (And if MayIncludeUndef is set, it's equivalent to markUndef().)
> > 
> > I tried to come up with cases where markUndef would be required for empty ranges but I couldn't, as the empty ranges should only occur in dead blocks. But I guess it is better to start out with being a bit more conservative.
> One thing to keep in mind here is that empty range can occur not only if the block is dead, but also if immediate UB is triggered (I believe you'll get an empty set from udiv by zero) and if an instruction always returns poison (e.g. due to nowrap flags -- SCCP is probably not wired up to this part, but LVI is). I don't think this changes anything about the conclusion here though.
> 
> If the behavior here is changed, this comment in LVI should also be updated: https://github.com/llvm/llvm-project/blob/45526d29a5b2cf126b83ada3991921970007d16f/llvm/lib/Analysis/LazyValueInfo.cpp#L125-L127
> Eli, do you by any chance remember why the restriction was there in the first place?

You mean, why we sent empty ranges to overdefined?  No memory of this.  Tried to dig a bit into blame, and I think it goes back to https://reviews.llvm.org/rL110388 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78667