[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:
> nikic wrote:
> > 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?
> > 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).
> 
> Yep, i was *not* suggesting special-casing single-element ranges.
> 
> > What would be the goal of the metric? To make sure that the "precision" never becomes lower if the implementation is changed?
> 
> That, or to gauge which operation needs more work.
> Just a thought, i'm not sure how useful it would be.
I gave the precision metric (with your suggested definition) a try and checked it against a couple of bit widths. This gave me 90% (3 bits), 95% (4 bits) and 97% (5 bits) precision.

Not sure what exactly that tells us though ^^ This is probably something that needs to be evaluated over the actual probability density of the input, otherwise we end up biasing heavily in favor of ranges with little practical relevance (at least one operand will the wrapping in 75% of the cases).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61207