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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/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)



================
Comment at: unittests/Analysis/LazyValueInfoTest.cpp:87
+  pred_iterator i = pred_begin(last);
+  BasicBlock *LHS = *i++;
+  BasicBlock *last = &*(--F->end());
----------------
This is not guaranteed to return the LHS block, fwiw :)



Repository:
  rL LLVM

https://reviews.llvm.org/D46279