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

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 5 09:36:25 PST 2019


nikic added a comment.

> Baseline ConstantRange::mul() does not have an exhaustive test,
>  and if i add one (TestUnsignedBinOpExhaustive()+/*CorrectnessOnly=*/true),
>  it doesn't pass.

For my own sanity: This doesn't mean that the multiply() implementation is incorrect, just that this testing function is a bit overly aggressive in CorrectnessOnly mode. It expects the result to be a superset of the minimal unsigned bounding range. We might want to change the function to actually only check correctness via `EXPECT_TRUE(CR.contains(N))`.

Looking a bit into what causes the failures for mulWithNoWrap when testing not only correctness for the unsigned case. I think there are two main causes:

- Something like `[0, 1] * [-8, -5]` produces `[-8, 0]`, while the testing code expects `[0, -5]`. This actual result is consistent with the "smallest range" preferred range mode, so this is just a testing deficiency.
- Something like `[0, 2] * [-8, -5]` produces full-set, while the testing code expects `[0, -5]`. The reason here is that the result can be in the ranges `[0, 0]`, `[-8, -5]` or `[2 * -8, 2 * -5]`, where the last range is unsigned (and signed) wrapping, so the result could actually be the same as for the previous case. However, we don't recognize that the `2 * [-8, -5]` case is fully excluded.

So I guess it would be pretty hard to make this work non-approximately...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70043





More information about the llvm-commits mailing list