Patchwork IVOPT improvement patch

login
register
mail settings
Submitter H.J. Lu
Date July 30, 2010, 8:43 p.m.
Message ID <AANLkTima5mV3OL=HuY_Sw54NtNU4-0RaY+z6jaNzA0Sc@mail.gmail.com>
Download mbox | patch
Permalink /patch/60382/
State New
Headers show

Comments

H.J. Lu - July 30, 2010, 8:43 p.m.
On Fri, Jul 30, 2010 at 11:46 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Jul 30, 2010 at 10:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Why is start offset not 1 to begin with? Let's assume it is correct,
>> there are a couple of problems in this patch:
>>
>> 1) when the precision of the HOST_WIDE_INT is the same as the bitsize
>> of the address_mode, max_offset = (HOST_WIDE_INT) 1 << width will
>> produce a negative number
>> 2) last_off should be initialized to 0 to match the original behavior
>> 3) The i&& guard will make sure the loop terminates, but the offset
>> compuation will be wrong -- i<<1 will first overflows to a negative
>> number, then gets truncated to zero,  that means when this happens,
>> the last_off will be negative when the loop terminates.
>>
>> David
>
> I don't know exactly what get_address_cost is supposed to do. Here is
> a new patch which avoids overflow and speeds up finding max/min offsets.
>


The code is wrong for -m32 on 64bit host. We should start with
the maximum and minimum offsets like:

      width = GET_MODE_BITSIZE (address_mode) - 1;
      if (width > (HOST_BITS_PER_WIDE_INT - 1))
        width = HOST_BITS_PER_WIDE_INT - 1;
      addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);

      for (i = width; i; i--)
        {
          off = -((HOST_WIDE_INT) 1 << i);
          XEXP (addr, 1) = gen_int_mode (off, address_mode);
          if (memory_address_addr_space_p (mem_mode, addr, as))
            break;
        }
      data->min_offset = off;

      for (i = width; i; i--)
        {
          off = ((HOST_WIDE_INT) 1 << i) - 1;
          XEXP (addr, 1) = gen_int_mode (off, address_mode);
          if (memory_address_addr_space_p (mem_mode, addr, as))
            break;
        }
      data->max_offset = off;

Here is the updated patch.


H.J.
---
> H.J.
> ---
>>
>> On Fri, Jul 30, 2010 at 10:27 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Fri, Jul 30, 2010 at 9:58 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> There is a problem in this patch -- when i wraps to zero and terminate
>>>> the loop, the maxoffset computed will be zero which is wrong.
>>>>
>>>> My previous patch won't have this problem.
>>>
>>> Your patch changed the start offset.  Here is the updated patch.
>>>
>>>
>>> H.J.
>>>>
>>>> David
>>>>
>>>> On Fri, Jul 30, 2010 at 9:49 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> This looks fine to me -- Zdenek or other reviewers --- is this one ok?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>> On Fri, Jul 30, 2010 at 8:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>> On Thu, Jul 29, 2010 at 6:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> It looks strange:
>>>>>>>
>>>>>>> +      width = (GET_MODE_BITSIZE (address_mode) <  HOST_BITS_PER_WIDE_INT - 1)
>>>>>>> +          ? GET_MODE_BITSIZE (address_mode) : HOST_BITS_PER_WIDE_INT - 1;
>>>>>>>       addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>>>>>>> -      for (i = start; i <= 1 << 20; i <<= 1)
>>>>>>> +      for (i = 1; i < width; i++)
>>>>>>>        {
>>>>>>> -         XEXP (addr, 1) = gen_int_mode (i, address_mode);
>>>>>>> +          HOST_WIDE_INT offset = (1ll << i);
>>>>>>> +         XEXP (addr, 1) = gen_int_mode (offset, address_mode);
>>>>>>>          if (!memory_address_addr_space_p (mem_mode, addr, as))
>>>>>>>            break;
>>>>>>>        }
>>>>>>>
>>>>>>> HOST_WIDE_INT may be long or long long. "1ll" isn't always correct.
>>>>>>> I think width can be >= 31. Depending on HOST_WIDE_INT,
>>>>>>>
>>>>>>> HOST_WIDE_INT offset = -(1ll << i);
>>>>>>>
>>>>>>> may have different values. The whole function looks odd to me.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Here is a different approach to check address overflow.
>>>>>>
>>>>>>
>>>>>> --
>>>>>> H.J.
>>>>>> --
>>>>>> 2010-07-29  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>>
>>>>>>        PR bootstrap/45119
>>>>>>        * tree-ssa-loop-ivopts.c (get_address_cost): Re-apply revision
>>>>>>        162652.  Check address overflow.
>>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> H.J.
>>>
>>
>
>
>
> --
> H.J.
>
Xinliang David Li - July 30, 2010, 8:52 p.m.
This final version looks good to me -- and it is more precise.

Thanks,

David

On Fri, Jul 30, 2010 at 1:43 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Jul 30, 2010 at 11:46 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Fri, Jul 30, 2010 at 10:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> Why is start offset not 1 to begin with? Let's assume it is correct,
>>> there are a couple of problems in this patch:
>>>
>>> 1) when the precision of the HOST_WIDE_INT is the same as the bitsize
>>> of the address_mode, max_offset = (HOST_WIDE_INT) 1 << width will
>>> produce a negative number
>>> 2) last_off should be initialized to 0 to match the original behavior
>>> 3) The i&& guard will make sure the loop terminates, but the offset
>>> compuation will be wrong -- i<<1 will first overflows to a negative
>>> number, then gets truncated to zero,  that means when this happens,
>>> the last_off will be negative when the loop terminates.
>>>
>>> David
>>
>> I don't know exactly what get_address_cost is supposed to do. Here is
>> a new patch which avoids overflow and speeds up finding max/min offsets.
>>
>
>
> The code is wrong for -m32 on 64bit host. We should start with
> the maximum and minimum offsets like:
>
>      width = GET_MODE_BITSIZE (address_mode) - 1;
>      if (width > (HOST_BITS_PER_WIDE_INT - 1))
>        width = HOST_BITS_PER_WIDE_INT - 1;
>      addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>
>      for (i = width; i; i--)
>        {
>          off = -((HOST_WIDE_INT) 1 << i);
>          XEXP (addr, 1) = gen_int_mode (off, address_mode);
>          if (memory_address_addr_space_p (mem_mode, addr, as))
>            break;
>        }
>      data->min_offset = off;
>
>      for (i = width; i; i--)
>        {
>          off = ((HOST_WIDE_INT) 1 << i) - 1;
>          XEXP (addr, 1) = gen_int_mode (off, address_mode);
>          if (memory_address_addr_space_p (mem_mode, addr, as))
>            break;
>        }
>      data->max_offset = off;
>
> Here is the updated patch.
>
>
> H.J.
> ---
>> H.J.
>> ---
>>>
>>> On Fri, Jul 30, 2010 at 10:27 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Fri, Jul 30, 2010 at 9:58 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> There is a problem in this patch -- when i wraps to zero and terminate
>>>>> the loop, the maxoffset computed will be zero which is wrong.
>>>>>
>>>>> My previous patch won't have this problem.
>>>>
>>>> Your patch changed the start offset.  Here is the updated patch.
>>>>
>>>>
>>>> H.J.
>>>>>
>>>>> David
>>>>>
>>>>> On Fri, Jul 30, 2010 at 9:49 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> This looks fine to me -- Zdenek or other reviewers --- is this one ok?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>> On Fri, Jul 30, 2010 at 8:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On Thu, Jul 29, 2010 at 6:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>> It looks strange:
>>>>>>>>
>>>>>>>> +      width = (GET_MODE_BITSIZE (address_mode) <  HOST_BITS_PER_WIDE_INT - 1)
>>>>>>>> +          ? GET_MODE_BITSIZE (address_mode) : HOST_BITS_PER_WIDE_INT - 1;
>>>>>>>>       addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>>>>>>>> -      for (i = start; i <= 1 << 20; i <<= 1)
>>>>>>>> +      for (i = 1; i < width; i++)
>>>>>>>>        {
>>>>>>>> -         XEXP (addr, 1) = gen_int_mode (i, address_mode);
>>>>>>>> +          HOST_WIDE_INT offset = (1ll << i);
>>>>>>>> +         XEXP (addr, 1) = gen_int_mode (offset, address_mode);
>>>>>>>>          if (!memory_address_addr_space_p (mem_mode, addr, as))
>>>>>>>>            break;
>>>>>>>>        }
>>>>>>>>
>>>>>>>> HOST_WIDE_INT may be long or long long. "1ll" isn't always correct.
>>>>>>>> I think width can be >= 31. Depending on HOST_WIDE_INT,
>>>>>>>>
>>>>>>>> HOST_WIDE_INT offset = -(1ll << i);
>>>>>>>>
>>>>>>>> may have different values. The whole function looks odd to me.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Here is a different approach to check address overflow.
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> H.J.
>>>>>>> --
>>>>>>> 2010-07-29  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>>>
>>>>>>>        PR bootstrap/45119
>>>>>>>        * tree-ssa-loop-ivopts.c (get_address_cost): Re-apply revision
>>>>>>>        162652.  Check address overflow.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> H.J.
>>>>
>>>
>>
>>
>>
>> --
>> H.J.
>>
>
>
>
> --
> H.J.
>
Xinliang David Li - Aug. 2, 2010, 9:23 p.m.
Compiler bootstrapped and tested with Lu's patch (with one minor
change to initialize off variable) (x86-64/linux) -- also checked dump
file that offsets are properly computed.

Ok for trunk?

Thanks,

David

On Fri, Jul 30, 2010 at 1:43 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Jul 30, 2010 at 11:46 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Fri, Jul 30, 2010 at 10:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> Why is start offset not 1 to begin with? Let's assume it is correct,
>>> there are a couple of problems in this patch:
>>>
>>> 1) when the precision of the HOST_WIDE_INT is the same as the bitsize
>>> of the address_mode, max_offset = (HOST_WIDE_INT) 1 << width will
>>> produce a negative number
>>> 2) last_off should be initialized to 0 to match the original behavior
>>> 3) The i&& guard will make sure the loop terminates, but the offset
>>> compuation will be wrong -- i<<1 will first overflows to a negative
>>> number, then gets truncated to zero,  that means when this happens,
>>> the last_off will be negative when the loop terminates.
>>>
>>> David
>>
>> I don't know exactly what get_address_cost is supposed to do. Here is
>> a new patch which avoids overflow and speeds up finding max/min offsets.
>>
>
>
> The code is wrong for -m32 on 64bit host. We should start with
> the maximum and minimum offsets like:
>
>      width = GET_MODE_BITSIZE (address_mode) - 1;
>      if (width > (HOST_BITS_PER_WIDE_INT - 1))
>        width = HOST_BITS_PER_WIDE_INT - 1;
>      addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>
>      for (i = width; i; i--)
>        {
>          off = -((HOST_WIDE_INT) 1 << i);
>          XEXP (addr, 1) = gen_int_mode (off, address_mode);
>          if (memory_address_addr_space_p (mem_mode, addr, as))
>            break;
>        }
>      data->min_offset = off;
>
>      for (i = width; i; i--)
>        {
>          off = ((HOST_WIDE_INT) 1 << i) - 1;
>          XEXP (addr, 1) = gen_int_mode (off, address_mode);
>          if (memory_address_addr_space_p (mem_mode, addr, as))
>            break;
>        }
>      data->max_offset = off;
>
> Here is the updated patch.
>
>
> H.J.
> ---
>> H.J.
>> ---
>>>
>>> On Fri, Jul 30, 2010 at 10:27 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Fri, Jul 30, 2010 at 9:58 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> There is a problem in this patch -- when i wraps to zero and terminate
>>>>> the loop, the maxoffset computed will be zero which is wrong.
>>>>>
>>>>> My previous patch won't have this problem.
>>>>
>>>> Your patch changed the start offset.  Here is the updated patch.
>>>>
>>>>
>>>> H.J.
>>>>>
>>>>> David
>>>>>
>>>>> On Fri, Jul 30, 2010 at 9:49 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> This looks fine to me -- Zdenek or other reviewers --- is this one ok?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>> On Fri, Jul 30, 2010 at 8:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On Thu, Jul 29, 2010 at 6:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>> It looks strange:
>>>>>>>>
>>>>>>>> +      width = (GET_MODE_BITSIZE (address_mode) <  HOST_BITS_PER_WIDE_INT - 1)
>>>>>>>> +          ? GET_MODE_BITSIZE (address_mode) : HOST_BITS_PER_WIDE_INT - 1;
>>>>>>>>       addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>>>>>>>> -      for (i = start; i <= 1 << 20; i <<= 1)
>>>>>>>> +      for (i = 1; i < width; i++)
>>>>>>>>        {
>>>>>>>> -         XEXP (addr, 1) = gen_int_mode (i, address_mode);
>>>>>>>> +          HOST_WIDE_INT offset = (1ll << i);
>>>>>>>> +         XEXP (addr, 1) = gen_int_mode (offset, address_mode);
>>>>>>>>          if (!memory_address_addr_space_p (mem_mode, addr, as))
>>>>>>>>            break;
>>>>>>>>        }
>>>>>>>>
>>>>>>>> HOST_WIDE_INT may be long or long long. "1ll" isn't always correct.
>>>>>>>> I think width can be >= 31. Depending on HOST_WIDE_INT,
>>>>>>>>
>>>>>>>> HOST_WIDE_INT offset = -(1ll << i);
>>>>>>>>
>>>>>>>> may have different values. The whole function looks odd to me.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Here is a different approach to check address overflow.
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> H.J.
>>>>>>> --
>>>>>>> 2010-07-29  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>>>
>>>>>>>        PR bootstrap/45119
>>>>>>>        * tree-ssa-loop-ivopts.c (get_address_cost): Re-apply revision
>>>>>>>        162652.  Check address overflow.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> H.J.
>>>>
>>>
>>
>>
>>
>> --
>> H.J.
>>
>
>
>
> --
> H.J.
>
Zdenek Dvorak - Aug. 9, 2010, 7:55 a.m.
Hi,

> Compiler bootstrapped and tested with Lu's patch (with one minor
> change to initialize off variable) (x86-64/linux) -- also checked dump
> file that offsets are properly computed.

in case that no offsets are allowed (or more hypotetically, if only offsets of
+1 or -1 are allowed), the code below will set min_offset to -2 and max_offset
to +2, thus incorrectly extending the range of allowed offsets.

Zdenek

>        reg1 = gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 1);
>  
> +      width = GET_MODE_BITSIZE (address_mode) - 1;
> +      if (width > (HOST_BITS_PER_WIDE_INT - 1))
> +	width = HOST_BITS_PER_WIDE_INT - 1;
>        addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
> -      for (i = start; i <= 1 << 20; i <<= 1)
> +
> +      for (i = width; i; i--)
>  	{
> -	  XEXP (addr, 1) = gen_int_mode (i, address_mode);
> -	  if (!memory_address_addr_space_p (mem_mode, addr, as))
> +	  off = -((HOST_WIDE_INT) 1 << i);
> +	  XEXP (addr, 1) = gen_int_mode (off, address_mode);
> +	  if (memory_address_addr_space_p (mem_mode, addr, as))
>  	    break;
>  	}
> -      data->max_offset = i == start ? 0 : i >> 1;
> -      off = data->max_offset;
> +      data->min_offset = off;
>  
> -      for (i = start; i <= 1 << 20; i <<= 1)
> +      for (i = width; i; i--)
>  	{
> -	  XEXP (addr, 1) = gen_int_mode (-i, address_mode);
> -	  if (!memory_address_addr_space_p (mem_mode, addr, as))
> +	  off = ((HOST_WIDE_INT) 1 << i) - 1;
> +	  XEXP (addr, 1) = gen_int_mode (off, address_mode);
> +	  if (memory_address_addr_space_p (mem_mode, addr, as))
>  	    break;
>  	}
> -      data->min_offset = i == start ? 0 : -(i >> 1);
> +      data->max_offset = off;
>  
>        if (dump_file && (dump_flags & TDF_DETAILS))
>  	{
>  	  fprintf (dump_file, "get_address_cost:\n");
> -	  fprintf (dump_file, "  min offset %s %d\n",
> +	  fprintf (dump_file, "  min offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
>  		   GET_MODE_NAME (mem_mode),
> -		   (int) data->min_offset);
> -	  fprintf (dump_file, "  max offset %s %d\n",
> +		   data->min_offset);
> +	  fprintf (dump_file, "  max offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
>  		   GET_MODE_NAME (mem_mode),
> -		   (int) data->max_offset);
> +		   data->max_offset);
>  	}
>  
>        rat = 1;
Xinliang David Li - Aug. 9, 2010, 10:54 p.m.
You are right. The attached is the revised version.  Ok this time
(after testing is done)?

Thanks,

David

On Mon, Aug 9, 2010 at 12:55 AM, Zdenek Dvorak <rakdver@kam.mff.cuni.cz> wrote:
> Hi,
>
>> Compiler bootstrapped and tested with Lu's patch (with one minor
>> change to initialize off variable) (x86-64/linux) -- also checked dump
>> file that offsets are properly computed.
>
> in case that no offsets are allowed (or more hypotetically, if only offsets of
> +1 or -1 are allowed), the code below will set min_offset to -2 and max_offset
> to +2, thus incorrectly extending the range of allowed offsets.
>
> Zdenek
>
>>        reg1 = gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 1);
>>
>> +      width = GET_MODE_BITSIZE (address_mode) - 1;
>> +      if (width > (HOST_BITS_PER_WIDE_INT - 1))
>> +     width = HOST_BITS_PER_WIDE_INT - 1;
>>        addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>> -      for (i = start; i <= 1 << 20; i <<= 1)
>> +
>> +      for (i = width; i; i--)
>>       {
>> -       XEXP (addr, 1) = gen_int_mode (i, address_mode);
>> -       if (!memory_address_addr_space_p (mem_mode, addr, as))
>> +       off = -((HOST_WIDE_INT) 1 << i);
>> +       XEXP (addr, 1) = gen_int_mode (off, address_mode);
>> +       if (memory_address_addr_space_p (mem_mode, addr, as))
>>           break;
>>       }
>> -      data->max_offset = i == start ? 0 : i >> 1;
>> -      off = data->max_offset;
>> +      data->min_offset = off;
>>
>> -      for (i = start; i <= 1 << 20; i <<= 1)
>> +      for (i = width; i; i--)
>>       {
>> -       XEXP (addr, 1) = gen_int_mode (-i, address_mode);
>> -       if (!memory_address_addr_space_p (mem_mode, addr, as))
>> +       off = ((HOST_WIDE_INT) 1 << i) - 1;
>> +       XEXP (addr, 1) = gen_int_mode (off, address_mode);
>> +       if (memory_address_addr_space_p (mem_mode, addr, as))
>>           break;
>>       }
>> -      data->min_offset = i == start ? 0 : -(i >> 1);
>> +      data->max_offset = off;
>>
>>        if (dump_file && (dump_flags & TDF_DETAILS))
>>       {
>>         fprintf (dump_file, "get_address_cost:\n");
>> -       fprintf (dump_file, "  min offset %s %d\n",
>> +       fprintf (dump_file, "  min offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
>>                  GET_MODE_NAME (mem_mode),
>> -                (int) data->min_offset);
>> -       fprintf (dump_file, "  max offset %s %d\n",
>> +                data->min_offset);
>> +       fprintf (dump_file, "  max offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
>>                  GET_MODE_NAME (mem_mode),
>> -                (int) data->max_offset);
>> +                data->max_offset);
>>       }
>>
>>        rat = 1;
>
>
Xinliang David Li - Aug. 9, 2010, 11:47 p.m.
Wrong patch in the last email. Here is the one.

David

On Mon, Aug 9, 2010 at 3:54 PM, Xinliang David Li <davidxl@google.com> wrote:
> You are right. The attached is the revised version.  Ok this time
> (after testing is done)?
>
> Thanks,
>
> David
>
> On Mon, Aug 9, 2010 at 12:55 AM, Zdenek Dvorak <rakdver@kam.mff.cuni.cz> wrote:
>> Hi,
>>
>>> Compiler bootstrapped and tested with Lu's patch (with one minor
>>> change to initialize off variable) (x86-64/linux) -- also checked dump
>>> file that offsets are properly computed.
>>
>> in case that no offsets are allowed (or more hypotetically, if only offsets of
>> +1 or -1 are allowed), the code below will set min_offset to -2 and max_offset
>> to +2, thus incorrectly extending the range of allowed offsets.
>>
>> Zdenek
>>
>>>        reg1 = gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 1);
>>>
>>> +      width = GET_MODE_BITSIZE (address_mode) - 1;
>>> +      if (width > (HOST_BITS_PER_WIDE_INT - 1))
>>> +     width = HOST_BITS_PER_WIDE_INT - 1;
>>>        addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
>>> -      for (i = start; i <= 1 << 20; i <<= 1)
>>> +
>>> +      for (i = width; i; i--)
>>>       {
>>> -       XEXP (addr, 1) = gen_int_mode (i, address_mode);
>>> -       if (!memory_address_addr_space_p (mem_mode, addr, as))
>>> +       off = -((HOST_WIDE_INT) 1 << i);
>>> +       XEXP (addr, 1) = gen_int_mode (off, address_mode);
>>> +       if (memory_address_addr_space_p (mem_mode, addr, as))
>>>           break;
>>>       }
>>> -      data->max_offset = i == start ? 0 : i >> 1;
>>> -      off = data->max_offset;
>>> +      data->min_offset = off;
>>>
>>> -      for (i = start; i <= 1 << 20; i <<= 1)
>>> +      for (i = width; i; i--)
>>>       {
>>> -       XEXP (addr, 1) = gen_int_mode (-i, address_mode);
>>> -       if (!memory_address_addr_space_p (mem_mode, addr, as))
>>> +       off = ((HOST_WIDE_INT) 1 << i) - 1;
>>> +       XEXP (addr, 1) = gen_int_mode (off, address_mode);
>>> +       if (memory_address_addr_space_p (mem_mode, addr, as))
>>>           break;
>>>       }
>>> -      data->min_offset = i == start ? 0 : -(i >> 1);
>>> +      data->max_offset = off;
>>>
>>>        if (dump_file && (dump_flags & TDF_DETAILS))
>>>       {
>>>         fprintf (dump_file, "get_address_cost:\n");
>>> -       fprintf (dump_file, "  min offset %s %d\n",
>>> +       fprintf (dump_file, "  min offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
>>>                  GET_MODE_NAME (mem_mode),
>>> -                (int) data->min_offset);
>>> -       fprintf (dump_file, "  max offset %s %d\n",
>>> +                data->min_offset);
>>> +       fprintf (dump_file, "  max offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
>>>                  GET_MODE_NAME (mem_mode),
>>> -                (int) data->max_offset);
>>> +                data->max_offset);
>>>       }
>>>
>>>        rat = 1;
>>
>>
>
Zdenek Dvorak - Aug. 10, 2010, 1:04 p.m.
Hi,

> Wrong patch in the last email. Here is the one.
> 
> David
> 
> On Mon, Aug 9, 2010 at 3:54 PM, Xinliang David Li <davidxl@google.com> wrote:
> > You are right. The attached is the revised version.  Ok this time
> > (after testing is done)?

OK,

Zdenek
H.J. Lu - Aug. 10, 2010, 1:16 p.m.
On Mon, Aug 9, 2010 at 4:47 PM, Xinliang David Li <davidxl@google.com> wrote:
> Wrong patch in the last email. Here is the one.
>

You changed the code from setting "off" to setting "offset":

-      data->min_offset = i == start ? 0 : -(i >> 1);
+      data->max_offset = (i == -1? 0 : off);
+      offset = data->max_offset;

"off" is used later:

3345               if (off_p)
3346                 base = gen_rtx_fmt_e (CONST, address_mode,
3347                                       gen_rtx_fmt_ee
3348                                         (PLUS, address_mode, base,
3349                                          gen_int_mode (off,
address_mode)))     ;
3350             }
3351           else if (off_p)
3352             base = gen_int_mode (off, address_mode);
3353           else

You can just add

off = 0;

before the loop. Then you can use

data->min_offset = off;
data->max_offset = off;

after the loop. It is faster.
H.J. Lu - Aug. 10, 2010, 2:12 p.m.
On Tue, Aug 10, 2010 at 6:16 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Aug 9, 2010 at 4:47 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Wrong patch in the last email. Here is the one.
>>
>
> You changed the code from setting "off" to setting "offset":
>
> -      data->min_offset = i == start ? 0 : -(i >> 1);
> +      data->max_offset = (i == -1? 0 : off);
> +      offset = data->max_offset;
>
> "off" is used later:
>
> 3345               if (off_p)
> 3346                 base = gen_rtx_fmt_e (CONST, address_mode,
> 3347                                       gen_rtx_fmt_ee
> 3348                                         (PLUS, address_mode, base,
> 3349                                          gen_int_mode (off,
> address_mode)))     ;
> 3350             }
> 3351           else if (off_p)
> 3352             base = gen_int_mode (off, address_mode);
> 3353           else
>
> You can just add
>
> off = 0;
>
> before the loop. Then you can use
>
> data->min_offset = off;
> data->max_offset = off;
>
> after the loop. It is faster.
>

Never mind this comment. But "off" is different from before.
Xinliang David Li - Aug. 10, 2010, 4:27 p.m.
Yes -- fixed the typo. Will retest and then commit.

THanks,

David

On Tue, Aug 10, 2010 at 7:12 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Aug 10, 2010 at 6:16 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Aug 9, 2010 at 4:47 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> Wrong patch in the last email. Here is the one.
>>>
>>
>> You changed the code from setting "off" to setting "offset":
>>
>> -      data->min_offset = i == start ? 0 : -(i >> 1);
>> +      data->max_offset = (i == -1? 0 : off);
>> +      offset = data->max_offset;
>>
>> "off" is used later:
>>
>> 3345               if (off_p)
>> 3346                 base = gen_rtx_fmt_e (CONST, address_mode,
>> 3347                                       gen_rtx_fmt_ee
>> 3348                                         (PLUS, address_mode, base,
>> 3349                                          gen_int_mode (off,
>> address_mode)))     ;
>> 3350             }
>> 3351           else if (off_p)
>> 3352             base = gen_int_mode (off, address_mode);
>> 3353           else
>>
>> You can just add
>>
>> off = 0;
>>
>> before the loop. Then you can use
>>
>> data->min_offset = off;
>> data->max_offset = off;
>>
>> after the loop. It is faster.
>>
>
> Never mind this comment. But "off" is different from before.
>
>
>
> --
> H.J.
>
H.J. Lu - Aug. 10, 2010, 4:36 p.m.
On Tue, Aug 10, 2010 at 9:27 AM, Xinliang David Li <davidxl@google.com> wrote:
> Yes -- fixed the typo. Will retest and then commit.
>

Can you post your patch?

Thanks.


> THanks,
>
> David
>
> On Tue, Aug 10, 2010 at 7:12 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Aug 10, 2010 at 6:16 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Mon, Aug 9, 2010 at 4:47 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> Wrong patch in the last email. Here is the one.
>>>>
>>>
>>> You changed the code from setting "off" to setting "offset":
>>>
>>> -      data->min_offset = i == start ? 0 : -(i >> 1);
>>> +      data->max_offset = (i == -1? 0 : off);
>>> +      offset = data->max_offset;
>>>
>>> "off" is used later:
>>>
>>> 3345               if (off_p)
>>> 3346                 base = gen_rtx_fmt_e (CONST, address_mode,
>>> 3347                                       gen_rtx_fmt_ee
>>> 3348                                         (PLUS, address_mode, base,
>>> 3349                                          gen_int_mode (off,
>>> address_mode)))     ;
>>> 3350             }
>>> 3351           else if (off_p)
>>> 3352             base = gen_int_mode (off, address_mode);
>>> 3353           else
>>>
>>> You can just add
>>>
>>> off = 0;
>>>
>>> before the loop. Then you can use
>>>
>>> data->min_offset = off;
>>> data->max_offset = off;
>>>
>>> after the loop. It is faster.
>>>
>>
>> Never mind this comment. But "off" is different from before.
>>
>>
>>
>> --
>> H.J.
>>
>
Xinliang David Li - Aug. 10, 2010, 5:09 p.m.
see attached.

David

On Tue, Aug 10, 2010 at 9:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Aug 10, 2010 at 9:27 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Yes -- fixed the typo. Will retest and then commit.
>>
>
> Can you post your patch?
>
> Thanks.
>
>
>> THanks,
>>
>> David
>>
>> On Tue, Aug 10, 2010 at 7:12 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Aug 10, 2010 at 6:16 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Mon, Aug 9, 2010 at 4:47 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> Wrong patch in the last email. Here is the one.
>>>>>
>>>>
>>>> You changed the code from setting "off" to setting "offset":
>>>>
>>>> -      data->min_offset = i == start ? 0 : -(i >> 1);
>>>> +      data->max_offset = (i == -1? 0 : off);
>>>> +      offset = data->max_offset;
>>>>
>>>> "off" is used later:
>>>>
>>>> 3345               if (off_p)
>>>> 3346                 base = gen_rtx_fmt_e (CONST, address_mode,
>>>> 3347                                       gen_rtx_fmt_ee
>>>> 3348                                         (PLUS, address_mode, base,
>>>> 3349                                          gen_int_mode (off,
>>>> address_mode)))     ;
>>>> 3350             }
>>>> 3351           else if (off_p)
>>>> 3352             base = gen_int_mode (off, address_mode);
>>>> 3353           else
>>>>
>>>> You can just add
>>>>
>>>> off = 0;
>>>>
>>>> before the loop. Then you can use
>>>>
>>>> data->min_offset = off;
>>>> data->max_offset = off;
>>>>
>>>> after the loop. It is faster.
>>>>
>>>
>>> Never mind this comment. But "off" is different from before.
>>>
>>>
>>>
>>> --
>>> H.J.
>>>
>>
>
>
>
> --
> H.J.
>
H.J. Lu - Aug. 10, 2010, 5:13 p.m.
On Tue, Aug 10, 2010 at 10:09 AM, Xinliang David Li <davidxl@google.com> wrote:
> see attached.
>
> David
>

You have

+      data->max_offset = (i == -1? 0 : off);
+      off = data->max_offset;

if (i == -1)
  off = 0;
data->max_offset; = off;

may  avoid one memory access.
Xinliang David Li - Aug. 10, 2010, 5:26 p.m.
Ok, if you insist on the perfection :)

David

On Tue, Aug 10, 2010 at 10:13 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Aug 10, 2010 at 10:09 AM, Xinliang David Li <davidxl@google.com> wrote:
>> see attached.
>>
>> David
>>
>
> You have
>
> +      data->max_offset = (i == -1? 0 : off);
> +      off = data->max_offset;
>
> if (i == -1)
>  off = 0;
> data->max_offset; = off;
>
> may  avoid one memory access.
>
> --
> H.J.
>
Xinliang David Li - Aug. 10, 2010, 11:55 p.m.
The committed patch also include a fix to a test case (make it more robust).

David

On Tue, Aug 10, 2010 at 10:26 AM, Xinliang David Li <davidxl@google.com> wrote:
> Ok, if you insist on the perfection :)
>
> David
>
> On Tue, Aug 10, 2010 at 10:13 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Aug 10, 2010 at 10:09 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> see attached.
>>>
>>> David
>>>
>>
>> You have
>>
>> +      data->max_offset = (i == -1? 0 : off);
>> +      off = data->max_offset;
>>
>> if (i == -1)
>>  off = 0;
>> data->max_offset; = off;
>>
>> may  avoid one memory access.
>>
>> --
>> H.J.
>>
>

Patch

diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
index 1d65b4a..b47acc7 100644
--- a/gcc/tree-ssa-loop-ivopts.c
+++ b/gcc/tree-ssa-loop-ivopts.c
@@ -3241,9 +3241,8 @@  get_address_cost (bool symbol_present, bool var_present,
   if (!data)
     {
       HOST_WIDE_INT i;
-      HOST_WIDE_INT start = BIGGEST_ALIGNMENT / BITS_PER_UNIT;
       HOST_WIDE_INT rat, off;
-      int old_cse_not_expected;
+      int old_cse_not_expected, width;
       unsigned sym_p, var_p, off_p, rat_p, add_c;
       rtx seq, addr, base;
       rtx reg0, reg1;
@@ -3252,33 +3251,38 @@  get_address_cost (bool symbol_present, bool var_present,
 
       reg1 = gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 1);
 
+      width = GET_MODE_BITSIZE (address_mode) - 1;
+      if (width > (HOST_BITS_PER_WIDE_INT - 1))
+	width = HOST_BITS_PER_WIDE_INT - 1;
       addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
-      for (i = start; i <= 1 << 20; i <<= 1)
+
+      for (i = width; i; i--)
 	{
-	  XEXP (addr, 1) = gen_int_mode (i, address_mode);
-	  if (!memory_address_addr_space_p (mem_mode, addr, as))
+	  off = -((HOST_WIDE_INT) 1 << i);
+	  XEXP (addr, 1) = gen_int_mode (off, address_mode);
+	  if (memory_address_addr_space_p (mem_mode, addr, as))
 	    break;
 	}
-      data->max_offset = i == start ? 0 : i >> 1;
-      off = data->max_offset;
+      data->min_offset = off;
 
-      for (i = start; i <= 1 << 20; i <<= 1)
+      for (i = width; i; i--)
 	{
-	  XEXP (addr, 1) = gen_int_mode (-i, address_mode);
-	  if (!memory_address_addr_space_p (mem_mode, addr, as))
+	  off = ((HOST_WIDE_INT) 1 << i) - 1;
+	  XEXP (addr, 1) = gen_int_mode (off, address_mode);
+	  if (memory_address_addr_space_p (mem_mode, addr, as))
 	    break;
 	}
-      data->min_offset = i == start ? 0 : -(i >> 1);
+      data->max_offset = off;
 
       if (dump_file && (dump_flags & TDF_DETAILS))
 	{
 	  fprintf (dump_file, "get_address_cost:\n");
-	  fprintf (dump_file, "  min offset %s %d\n",
+	  fprintf (dump_file, "  min offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
 		   GET_MODE_NAME (mem_mode),
-		   (int) data->min_offset);
-	  fprintf (dump_file, "  max offset %s %d\n",
+		   data->min_offset);
+	  fprintf (dump_file, "  max offset %s " HOST_WIDE_INT_PRINT_DEC "\n",
 		   GET_MODE_NAME (mem_mode),
-		   (int) data->max_offset);
+		   data->max_offset);
 	}
 
       rat = 1;