[PATCH] D75120: [ValueLattice] Add new state for undef constants.

Wed Mar 11 17:03:57 PDT 2020

efriedma added inline comments.

================
Comment at: llvm/include/llvm/Analysis/ValueLattice.h:137
+    if (isa<UndefValue>(C))
+      Res.markUndefined();
+    else
----------------
fhahn wrote:
> efriedma wrote:
> > fhahn wrote:
> > > efriedma wrote:
> > > > I'm not sure I understand how getNot(undef) is supposed to work.
> > > I think it should be fine to assume `x != undef => undef`. Alive seems to agree. Granted, it is probably not very useful. I tried to replace it with an assert ruling out undef here, but we hit this case in practice in LVI via JumpThreading.
> > If you prove x != undef, x can't be any specific value... including undef, I think.  So actually you could just treat it as unknown, I think?
> Looking at the uses in LVI, it seems like it is used for information coming from things like `%x = icmp ne %a, CONST; %y = select %x, %a, 10` and LVI would use `getNot(CONST)` and use that to mark %y as `!= CONST` if `10 != Const` . (https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/LazyValueInfo.cpp#L953) 
> 
> I'm not sure if that allows saying that `%a` cannot be any specific value including undef, if `CONST` is undef. Couldn't the use in `%a` be any value, by choosing something like `%a+1` for undef in the icmp? 
> 
> Because IIUC we can only chose a single concrete value for the undef in the icmp per execution. 
> 
> Another way to think about it might be a constant range with a single value missing. The missing value can be arbitrarily chosen per use/execution. I think what's allowed and dis-allowed would heavily depend on how it is used exactly. Maybe it would be best to start out with overdefined in this patch and refine it if required later on?
Oh, I see, code being analyzed is not actually assuming it like an llvm.assume; it's just using it in the condition of a select.

I guess that `a == undef ? 0 : undef+1` is in fact undef... but I don't think that generalizes in any useful way to other places we might use getNot().  I'd prefer to explicitly bail out in LVI, as opposed to handling it here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75120