[PATCH] D142330: [AssumptionCache] caches @llvm.experimental.guard's

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

If this change goes forward, we need to investigate all places where assumes/AssumptionCache is used and see if what is being done for assumes holds for guards as well. In LVI, what was done for assumes did not hold for guards. 
Also, in future, when anyone adds assumption cache (under the idea that it handles assumes), we can very easily fall into the problem of having a miscompile with guards. For that reason and given that we are working towards moving away from this implementation, I would suggest we abandon this change altogether.

For the details, here is the code where we miscompile in LVI:

  void LazyValueInfoImpl::intersectAssumeOrGuardBlockValueConstantRange(
            Value *Val, ValueLatticeElement &BBLV, Instruction *BBI) {
  ...
  BasicBlock *BB = BBI->getParent();
      for (auto &AssumeVH : AC->assumptionsFor(Val)) {
        if (!AssumeVH)
          continue;
    
        // Only check assumes in the block of the context instruction. Other
        // assumes will have already been taken into account when the value was
        // propagated from predecessor blocks.
        auto *I = cast<CallInst>(AssumeVH);
        if (I->getParent() != BB || !isValidAssumeForContext(I, BBI))
          continue;
    
        BBLV = intersect(BBLV, getValueFromCondition(Val, I->getArgOperand(0))); <-- we were intersecting with a guard here (!) without checking where the guard belonged 
      }
    
      // If guards are not used in the module, don't spend time looking for them
      if (GuardDecl && !GuardDecl->use_empty() &&
          BBI->getIterator() != BB->begin()) {
        for (Instruction &I : make_range(std::next(BBI->getIterator().getReverse()),
                                         BB->rend())) {
          Value *Cond = nullptr;
          if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>(m_Value(Cond))))
            BBLV = intersect(BBLV, getValueFromCondition(Val, Cond));
        }
      }
    
  ..
  }

Note that with guards we intersect LVI by reverse traversing from the context instruction upwards till start of basic block. The assumption intersection above it does not do that - it just takes all assumes in the block. `isValidAssumeForContext` allows assumes which are after the context instruction as long as we will reach that assume (this would be incorrect in the case of guards). With this patch, we were intersecting the constant range with a guard's argument which was after `Val` and incorrectly folding a compare in correlated-value-propagation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142330