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

[James Molloy]

Hi all,

Thanks for picking up on this. It’s not as obvious as I first thought. Hal, yes I was just about to say we need to take the minimum of the cartesian product minus maxA*maxB, which is obviously never going to give a minimum. Inverse for the maximum.

Sanjoy, I’m not entirely sure - it’s a bit late for me to have my effective thinking cap on. At the moment it does seem to me that if we swapped ZExt for Sext in multiply() things might just come out correctly, but I’d need to prove it to myself.

James

On 20 Feb 2015, at 23:48, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:

> 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.
>
> -- 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
>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782