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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Analysis/ValueLattice.h:359
+        return false;
       return markOverdefined();
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78667