[PATCH] D61314: [SCCP] Remove forcedconstant, go to overdefined instead

Mon Feb 17 14:36:36 PST 2020

fhahn added a comment.

In D61314#1879602 <https://reviews.llvm.org/D61314#1879602>, @efriedma wrote:

> I was thinking about this a bit more, and I'm not sure this is enough to allow value ranges to work correctly around undef.
>
> The problem essentially boils down to this: what happens when an instruction that might return undef is marked "constantrange"?  If an instruction is marked "constant", it doesn't really matter if it really returns "constant or undef": SCCP will RAUW the instruction away, so it can't produce undef after SCCP runs.  If it's a constantrange, though, we never RAUW the value, so the "undef" case never goes away.  And if it's undef, it could be outside the computed range, and we could drop necessary arithmetic/comparisons.  For example, suppose we compute a value X is in range 0-255, and then we "and X, 255".  It looks like we could drop the "and"... but we never actually eliminated the possibility that the value was undef.  So now we've transformed "and undef, 255" -> "undef", which is wrong.

I thought a bit more about the cases where we generate ranges:

1. Ops with all operands either constant/constant range (e.g. add, sub, mul, select)
2. Range restricting ops with overdefined, constant/constant range operands (e.g. and overdefined, [0, 255])
3. Phis with multiple live incoming values
4. Function arguments with multiple call sites.

I think for 1. and 2., we should never generate a concrete range, if any of the operands may be undef as in your example. As long as any operand is unknown/undef, we wait until ResolvedUndefsIn.

In 3. and 4. we could however generate concrete ranges when one of the incoming values/concrete arguments may be undef. For phi nodes for example, the current implementation just skips incoming unknown/undefined values. If one of the incoming values is a range 0-255 and the other incoming value is undef, we end up with a value X as you described. I think we could however handle this directly in visitPhiNode: if some of the incoming values are unknown and some others are non-singleton constant ranges we have to go to overdefined.

I think we can deal with 4. in a similar fashion: when merging function arguments, we need to go to overdefined once we merge in an unknown/undef value and a (non singleton) constant range.

I think that should ensure we never construct a concrete range for a value that might be undef. I hope I didn't miss any cases. What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61314