[PATCH] D97077: [SCEV] Pass an explicit context to computeKnownBits

Fri Feb 19 20:03:30 PST 2021

reames added a comment.

(re-post of an earlier comment with mistakes fixed and more context added)

In D97077#2575360 <https://reviews.llvm.org/D97077#2575360>, @nikic wrote:

> I don't think this is solving the problem in the right place -- this is a general `computeKnownBits()` issue, not specific to SCEV. See D93974 <https://reviews.llvm.org/D93974> for an alternative approach, and the discussion there regarding how to avoid the ephemeral value issue.

These are definitely related, but not, I think, directly the same.  I'd actually started with an approach very similar to that review before switching course.  I went back and cleaned up what I had.  That's now posted as D97099 <https://reviews.llvm.org/D97099>.

Right now, I see three related issues: 1) arguments should use the first instruction in the function as context, 2) non-instruction values (such as globals!) should use the first instruction in the function as context, and 3) we should not bail out on an assume which is the context instruction.

The linked patch tries to target (1) and (3).  This patch specifically only targets (1) and and maybe (2).  The choice not to include (3) was very deliberate to allow step wise progress.  We do need a generic solution to (3), but it shouldn't block (1) or (2).

On (2), this patch does not handle that case.  It could - and to be honest, I originally thought it did - but as shown by the test case for constant expressions it doesn't currently work out.  I'll will note that (2) can not (in general) be done inside computeKnownBits.  Why?  Because we don't know the function context it's invoked in.  (Though, we might be able to play some games with the analyzes as dom tree and assumption cache are tied to a single function scope.)

I believe we should move forward with either this patch or D97099 <https://reviews.llvm.org/D97099> in the near term.  Both are surgical improvements, and don't require solving the (fairly hard) item #3.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97077