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

Tue Feb 25 08:35:31 PST 2020

fhahn added a comment.

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

> > presumably because we skip incoming values from unreachable blocks in PHIs
>
> That doesn't make any sense.  We should skip incoming values from unreachable blocks no matter what they contain. I assume the significant difference between the two approaches is that one is special-casing merging undef and a constant.

Ah yes, my wording got mixed up here, sorry! The point I wanted to make is that we seem to not loose much in terms of precision by mergeIn(ConstantRange, unknown) -> overdefined.

> I'm curious about two other possibilities:
> 
> 1. Use overdefined for undef constants, but add a special case for PHI nodes with a literal "undef" operand.

This seems to have no impact. I've changed `getValueState` to mark UndefValue constants as overdefined and then added a special case to visitPHINode, which skips incoming UndefValues if the PHI has a constant value. getValueState. (D75122 <https://reviews.llvm.org/D75122>) I think should be along the lines you suggested here.

> 2. Treat undef as if it were an opaque constant, like a constant expression you can't analyze, instead of going straight to overdefined, then again add a special case for PHI nodes with a constant undef operand.

I started with this approach, but it seemed like it would require updating lots of places where we handle constants which do not expect it to be undef.

I've tried a slightly different approach (see D75120 <https://reviews.llvm.org/D75120>), which I think is quite similar in spirit: add a new undef lattice state, which can be merged with constants, but merging it with non-singleton ranges goes to overdefined. I think that makes it easier to update the code. Most places in SCCP can treat it similar to unknown, in that we can delay marking instructions with undef operands as overdefined until we finished solving. Is that approach roughly along the direction you suggested? For cases like `%p = phi [undef], [0]` `%foo = add %p, 1`, it is important to not mark `%foo` overdefined before we merged all incoming values of `%p`. Without delaying solving instructions with undef operands, we would have to mark `%foo` overdefined once we have `%p == undef`.

Also, from the comments in ValueLattice.h and in LVI, it seems the code expects unknown to mean unreachable/dead, which is slightly different to undef and the new state would help to clarify this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61314