[PATCH] D112637: [SCEVExpander] Be more conservative about poison flags when reusing instructions

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1855
+    // alternate form.  Thus, we must conclude that S conceptually has
+    // some unknown set of flags and thus that I must contradict them.
+    return false;
----------------
This comment confuses me. We cannot have used the flags on the binop unless they always hold (via poison UB reasoning). The conclusion is still correct in that we simply don't know whether the flags are always valid or not.

However, the problem goes beyond this. Just because you have some OBO and some SCEV with nowrap flags, does not mean that these flags have the same meaning. For example, consider
```
%a = add %x, 2
%b = add nuw %a, -1
```
in a context where `%b` is unused and `%x` has known range `[0,-2]`. The SCEV for `%b` is going to be `%x +<nuw> 1` after folding the adds together and using range-based flag strengthening. However, the `add nuw %a, -1` is clearly going to unsigned wrap for all values of `%x` other than -2. The problem here is that while both the IR instruction and the SCEV have the `nuw` flag, they also have different operands. One is `%a - 1`, the other is `%x + 1`.

The "proper" way to determine that the wrap flags really hold is to do the `ext(x + y) == ext(x)  + ext(y)` dance, but that's really expensive and I'm not sure it's desirable to created these kinds of expression in the expander (especially as they have side-effects on nowrap flags).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112637