Sure. I will submit a patch. BTW, what's the difference between the bounds I was expecting APInt NewLower = getLower() - Other.getUpper() + 1; APInt NewUpper = getUpper() - Other.getLower(); and the two you mentioned NewLower = Lower - (Upper-1) NewUpper = (Upper-1) - Lower + 1 They look equivalent to me. Did I miss anything? Thanks. - xi On Jun 22, 2011, at 2:39 PM, Nick Lewycky wrote:> Thanks, I think you've found a serious bug! > > Would you be willing to fix it? Please add a test to unittests/Support/ConstantRangeTest.cpp and then mail llvm-commits with the patch to fix it and add the test. > > On 20 June 2011 23:09, Xi Wang <xi.wang at gmail.com> wrote: > Hi, > > I have a question about ConstantRange::sub(const ConstantRange &Other) at lib/Support/ConstantRange.cpp:524. The code computes the new bounds as follows. > > APInt NewLower = getLower() - Other.getLower(); > APInt NewUpper = getUpper() - Other.getUpper() + 1; > > Could someone explain this to me? I was expecting something like > > APInt NewLower = getLower() - Other.getUpper() + 1; > APInt NewUpper = getUpper() - Other.getLower(); > > These aren't quite right, I think it should be: > > NewLower = Lower - (Upper-1) > NewUpper = (Upper-1) - Lower + 1 > > Constant ranges are stored half-open, [lower, upper) which means that the upper value is one past the end of the range. I often think of the formula as newmax = max - min --> newupper - 1 = ((getUpper() - 1) - Other.getLower(). min = lower, while max = upper - 1. > > Nick
On 22 June 2011 12:51, Xi Wang <xi.wang at gmail.com> wrote:> Sure. I will submit a patch. > > BTW, what's the difference between the bounds I was expecting > > APInt NewLower = getLower() - Other.getUpper() + 1; > APInt NewUpper = getUpper() - Other.getLower(); > > and the two you mentioned > > NewLower = Lower - (Upper-1) > NewUpper = (Upper-1) - Lower + 1 > > They look equivalent to me. Did I miss anything? Thanks. >... they are. Sorry, I was just surprised to see the +1 on the NewLower (it always goes on the NewUpper instead, right), but I didn't actually simplify the expressions. It sounds to me like you've got this one handled. :-) Nick> > - xi > > On Jun 22, 2011, at 2:39 PM, Nick Lewycky wrote: > > > Thanks, I think you've found a serious bug! > > > > Would you be willing to fix it? Please add a test to > unittests/Support/ConstantRangeTest.cpp and then mail llvm-commits with the > patch to fix it and add the test. > > > > On 20 June 2011 23:09, Xi Wang <xi.wang at gmail.com> wrote: > > Hi, > > > > I have a question about ConstantRange::sub(const ConstantRange &Other) at > lib/Support/ConstantRange.cpp:524. The code computes the new bounds as > follows. > > > > APInt NewLower = getLower() - Other.getLower(); > > APInt NewUpper = getUpper() - Other.getUpper() + 1; > > > > Could someone explain this to me? I was expecting something like > > > > APInt NewLower = getLower() - Other.getUpper() + 1; > > APInt NewUpper = getUpper() - Other.getLower(); > > > > These aren't quite right, I think it should be: > > > > NewLower = Lower - (Upper-1) > > NewUpper = (Upper-1) - Lower + 1 > > > > Constant ranges are stored half-open, [lower, upper) which means that the > upper value is one past the end of the range. I often think of the formula > as newmax = max - min --> newupper - 1 = ((getUpper() - 1) - > Other.getLower(). min = lower, while max = upper - 1. > > > > Nick > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20110622/032ffc9f/attachment.html>
Here goes the patch along with a test for ConstantRange::sub. Thanks. -------------- next part -------------- A non-text attachment was scrubbed... Name: crsub.patch Type: application/octet-stream Size: 1293 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20110622/bf6f65b9/attachment.obj> -------------- next part -------------- On Jun 22, 2011, at 3:57 PM, Nick Lewycky wrote:> On 22 June 2011 12:51, Xi Wang <xi.wang at gmail.com> wrote: > Sure. I will submit a patch. > > BTW, what's the difference between the bounds I was expecting > > APInt NewLower = getLower() - Other.getUpper() + 1; > APInt NewUpper = getUpper() - Other.getLower(); > > and the two you mentioned > > NewLower = Lower - (Upper-1) > NewUpper = (Upper-1) - Lower + 1 > > They look equivalent to me. Did I miss anything? Thanks. > > ... they are. Sorry, I was just surprised to see the +1 on the NewLower (it always goes on the NewUpper instead, right), but I didn't actually simplify the expressions. > > It sounds to me like you've got this one handled. :-) > > Nick > > > - xi > > On Jun 22, 2011, at 2:39 PM, Nick Lewycky wrote: > > > Thanks, I think you've found a serious bug! > > > > Would you be willing to fix it? Please add a test to unittests/Support/ConstantRangeTest.cpp and then mail llvm-commits with the patch to fix it and add the test. > > > > On 20 June 2011 23:09, Xi Wang <xi.wang at gmail.com> wrote: > > Hi, > > > > I have a question about ConstantRange::sub(const ConstantRange &Other) at lib/Support/ConstantRange.cpp:524. The code computes the new bounds as follows. > > > > APInt NewLower = getLower() - Other.getLower(); > > APInt NewUpper = getUpper() - Other.getUpper() + 1; > > > > Could someone explain this to me? I was expecting something like > > > > APInt NewLower = getLower() - Other.getUpper() + 1; > > APInt NewUpper = getUpper() - Other.getLower(); > > > > These aren't quite right, I think it should be: > > > > NewLower = Lower - (Upper-1) > > NewUpper = (Upper-1) - Lower + 1 > > > > Constant ranges are stored half-open, [lower, upper) which means that the upper value is one past the end of the range. I often think of the formula as newmax = max - min --> newupper - 1 = ((getUpper() - 1) - Other.getLower(). min = lower, while max = upper - 1. > > > > Nick > >