[PATCH] D61207: [ConstantRange] Add srem() support

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:127
+    ConstantRange Exact = ConstantRange::getNonEmpty(Min, Max + 1);
+    if (CorrectnessOnly) {
+      EXPECT_TRUE(CR.contains(Exact));
----------------
lebedev.ri wrote:
> Hm, i wonder if we can also check that the result
> is exact when the ranges contain a single element?
> Not sure if that would hold for the current implementations though.
> 
> I also wonder if some kind of a "precision" metric should be calculated,
> e.g. `ConstantRange` returned range with 75 elements, and exhaustive test
> returned range with 67 elements then let's say it's 67/75.
> And then just average that metric over all the checked ranges.
> Hm, i wonder if we can also check that the result
> is exact when the ranges contain a single element?
> Not sure if that would hold for the current implementations though.

Unfortunately the current implementation (also the urem implementation) is not exact for single element ranges, even if both ranges only have a single element. For example `15 % 10` will return `[0, 9]` instead of `5`.

The basic problem is that if the LHS is larger than the RHS, we have to perform a modular reduction so that `[12, 18] % 10` maps onto `[2, 8]`. However, if both ends of the range do not divide to the same value, then we get back the full modulus range, such as `[18, 22] % 10` being `[0, 9]`, because `18 / 10 == 1` while `24 / 10 == 2`.

For a single-element modulus this would be easy enough to handle, but once we get a proper range, I'm not sure how to do this, or if it's even possible to do it efficiently.

Special handling for the single-element case would definitely be possible, I'm just not sure if it's a good idea (I don't think I've seen any of the other constant range code do that).

> I also wonder if some kind of a "precision" metric should be calculated,
> e.g. ConstantRange returned range with 75 elements, and exhaustive test
> returned range with 67 elements then let's say it's 67/75.
> And then just average that metric over all the checked ranges.

What would be the goal of the metric? To make sure that the "precision" never becomes lower if the implementation is changed?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61207