[PATCH] D70043: [ConstantRange] Add `mulWithNoWrap()` method

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1003
+    for (const APInt &L : {getUnsignedMin(), getUnsignedMax()}) {
+      for (const APInt &R : {Other.getUnsignedMin(), Other.getUnsignedMax()}) {
+        bool Overflow;
----------------
nikic wrote:
> Why do these fetch the unsigned min/max for the signed multiplication case?
Right, this makes little sense.
I just ad-hoc-ked it because that is what worked, and it was likely wrong from precision standpoint.
I think this is more obvious now? (Did i horrendously over-engineer this and overlooking existing infra?)


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1022
+
+    if (NoWrapKind & OBO::NoSignedWrap) {
+      (void)getUnsignedMin().smul_ov(Other.getUnsignedMin(), Overflow);
----------------
nikic wrote:
> Why do we need to specially handle the nuw+nsw combination here?
Even after fixing `OBO::NoSignedWrap` check this remains.
Here in `OBO::NoUnsignedWrap` we did check that
`getUnsignedMin() * Other.getUnsignedMin()`
does not exhibit *unsigned* overflow,
but we did not do any such check in `OBO::NoSignedWrap`.

There, even if `UnsignedMin` happens to be one of the values closest to 0,
since we check that all values overflow, unless said range only contains
a single `UnsignedMin`, we wouldn't discard such range as always-overflowing.

So i **think** this is correct.
At least until precision tests say otherwise.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70043