[PATCH] D97092: [ValueTracking] Handle assumes on arguments with context instruction

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D97092#2576234 <https://reviews.llvm.org/D97092#2576234>, @reames wrote:

> As noted previously, when analyzing an assume, we must pass in a context instruction to prevent using the assumption in forming facts about the conditions contributing to the assume (when that assume is in the entry block and involves an argument).  The problem is that SCEV does not do so, and in fact, can not.  As a result, SCEV will infer facts about the values which hold because of the assume.  If we can trick any SCEV based transform pass into optimizing the assume condition, we've created the exact condition that the ephemeral logic code exists to prevent.

You're correct, but I don't think you've followed this thought to its logical conclusion. The difference between the different approaches we have isn't whether ephemeral values get ignored, it's whether they get ignored intentionally, or accidentally.

  foo(int a, int b) {
      assume(a != 0);
      assume(b != 0);
  }
  foo(int a, int b) {
      assume(b != 0);
      assume(a != 0);
  }

This patch is going to use both assumption consistently. Approaches that set context to the first instruction will use the second assume in each function, but not the first. In either case, if the analysis result is used to fold the assume conditions, we may end up eliminating non-redundant assumes.

This is a problem that will affect **any** approach that uses assumes on the value itself without an explicit context instruction that guarantees that assume conditions will not get folded. And I think you're right that we should avoid this. That does mean that all four patches are ultimately broken (heh).

For SCEV, the right approach would probably be to do something similar to LVI, and take assumptions into account not directly on the value itself, but on each use, i.e. `assume(x >= 0); x >> 1` would apply the assume only when computing the range of `x >> 1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97092