[PATCH] [ConstantRange] Add a smultiply method for signed multiply

[Hal Finkel]

----- Original Message -----
> From: "Sanjoy Das" <sanjoy at playingwithpointers.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "james molloy" <james.molloy at arm.com>, "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>,
> "reviews+d7789+public+ffa7549bb8a7b441" <reviews+D7789+public+ffa7549bb8a7b441 at reviews.llvm.org>
> Sent: Friday, February 20, 2015 5:48:42 PM
> Subject: Re: [PATCH] [ConstantRange] Add a smultiply method for signed multiply
> 
> Actually, I don't think there is any need for a separate smultiply
> when we're working on two's complement.  IMHO, the right solution is
> to just make ConstantRange::multiply smarter about handing cases like
> [-1,2) * [-1,2) etc.

Can this be rephased as saying that we should make ConstantRange::multiply smarter, generally speaking. Right now, it ignores the ranges and uses only the full representable widths of the inputs.

 -Hal

> 
> -- Sanjoy
> 
> On Fri, Feb 20, 2015 at 3:40 PM, Sanjoy Das
> <sanjoy at playingwithpointers.com> wrote:
> > I don't think so:
> >
> > [-3,0) * [-3, 0) should be [1, 10) but using your formula I get [3,
> > <something>).
> >
> > On Fri, Feb 20, 2015 at 3:31 PM, Hal Finkel <hfinkel at anl.gov>
> > wrote:
> >> ----- Original Message -----
> >>> From: "Sanjoy Das" <sanjoy at playingwithpointers.com>
> >>> To: reviews+D7789+public+ffa7549bb8a7b441 at reviews.llvm.org
> >>> Cc: "james molloy" <james.molloy at arm.com>, "Hal Finkel"
> >>> <hfinkel at anl.gov>, "Commit Messages and Patches for LLVM"
> >>> <llvm-commits at cs.uiuc.edu>
> >>> Sent: Friday, February 20, 2015 5:11:46 PM
> >>> Subject: Re: [PATCH] [ConstantRange] Add a smultiply method for
> >>> signed multiply
> >>>
> >>> >  ConstantRange
> >>> > +ConstantRange::smultiply(const ConstantRange &Other) const {
> >>> > +  // TODO: If either operand is a single element and the
> >>> > multiply
> >>> > is known to
> >>> > +  // be non-wrapping, round the result min and max value to
> >>> > the
> >>> > appropriate
> >>> > +  // multiple of that element. If wrapping is possible, at
> >>> > least
> >>> > adjust the
> >>> > +  // range according to the greatest power-of-two factor of
> >>> > the
> >>> > single element.
> >>> > +
> >>> > +  if (isEmptySet() || Other.isEmptySet())
> >>> > +    return ConstantRange(getBitWidth(), /*isFullSet=*/false);
> >>> > +
> >>> > +  APInt this_min = getSignedMin().sext(getBitWidth() * 2);
> >>> > +  APInt this_max = getSignedMax().sext(getBitWidth() * 2);
> >>> > +  APInt Other_min = Other.getSignedMin().sext(getBitWidth() *
> >>> > 2);
> >>> > +  APInt Other_max = Other.getSignedMax().sext(getBitWidth() *
> >>> > 2);
> >>> > +
> >>> > +  ConstantRange Result_zext = ConstantRange(this_min *
> >>> > Other_min,
> >>> > +                                            this_max *
> >>> > Other_max +
> >>> > 1);
> >>> > +  return Result_zext.truncate(getBitWidth());
> >>>
> >>> I'm not sure if this is correct:  if we multiply [-1, 2) with
> >>> itself,
> >>> won't this return [1, 2)?  Shouldn't [-1,2) * [-1,2) be [-1,2)?
> >>
> >> Indeed. The minimum should not be this_min * Other_min, but rather
> >> min(this_min * Other_max, this_max * Other_min), right?
> >>
> >>  -Hal
> >>
> >>>
> >>> -- Sanjoy
> >>>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory