[LVI] Remove an assert on case which could happen in LazyValueInfo.

[trentxintong]

Summary

If an instruction is defined in a block not the the one we try to get
the value in. We will keep chasing up the predecessors till we figure
out a value for the Val*.

This, however, can take us up to the entry block even though we are
not looking at a function argment.

[reames]

The assertion is checking the a value is not live-in to the entry block unless it is a global. That assert is correct. From your test case, it looks like you've actually found a place where we recurse on a non-local search we shouldn't have. (i.e. a local add with two constant operands should never trigger the backwards walk.)

p.s. You can probably write your test as an IR test.

[• dberlin]

unittests/Analysis/LazyValueInfoTest.cpp

78

FWIW: This is almost certainly easier to just do with Builder rather than parsing assembly strings :)

[• dberlin]

In D46279#1084112, @reames wrote:

The assertion is checking the a value is not live-in to the entry block unless it is a global. That assert is correct. From your test case, it looks like you've actually found a place where we recurse on a non-local search we shouldn't have. (i.e. a local add with two constant operands should never trigger the backwards walk.)

Dunno if you noticed this, but this is because they are specifically asking about the local value at the end of a block that comes before it (IE a block not dominated by the definition of that value).

The add is in block "last", and they are asking for the value of that add in block "lhs".

This is a nonsensical query in the first place, there is no value for the add in block "last".

LVI should, i believe, simply return null here, because the specific value is not known to be constant at the end of the specific block:

"

/// Determine whether the specified value is known to be a
/// constant at the end of the specified block.  Return null if not.
Constant *getConstant(Value *V, BasicBlock *BB, Instruction *CxtI = nullptr);

"

If you ask LVI the query LVI->getConstant(I, Last), it correctly gives the value "20".

The other possibility is to declare this query invalid, since it requires this interface dominance check that V->getParent() dominates BB on every call and return nullptr if false.
Then we just add an assert about it and fix any bugs.
IMHO, unclear that it's worth paying the cost at all, even constant time, for making this query return nullptr. I'm also surethat most calls trying to ask this question are bugs.
While I can think of asking LVI this kind of question in PRE or something where it wants to know what the value would be in a previous block, IMHO it would be better to have a different API where that kind of query is valid and does something sensible (AFAICT, it will not do anything sensible here, like phi translate the operands).

unittests/Analysis/LazyValueInfoTest.cpp

87

This is not guaranteed to return the LHS block, fwiw :)

[trentxintong]

@dberlin @reames I completely agree with the diagnosis and the proposed solution. Let me fix this.

[trentxintong]

And I think adding an assert to check against queries w.r.t. dominance makes sense.