| Message ID | AANLkTikX01vs8_taOwuEnULBkXaRhr1rQHQR4KUMXuM7@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. >
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. >
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. 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. >> >
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. >>> >> >
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 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. >
diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c index 1d65b4a..55aa10c 100644 --- a/gcc/tree-ssa-loop-ivopts.c +++ b/gcc/tree-ssa-loop-ivopts.c @@ -3243,7 +3243,7 @@ get_address_cost (bool symbol_present, bool var_present, 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,8 +3252,10 @@ 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) < HOST_BITS_PER_WIDE_INT - 2) + ? GET_MODE_BITSIZE (address_mode) : HOST_BITS_PER_WIDE_INT - 2; addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX); - for (i = start; i <= 1 << 20; i <<= 1) + for (i = start; i && i <= (HOST_WIDE_INT) 1 << width; i <<= 1) { XEXP (addr, 1) = gen_int_mode (i, address_mode); if (!memory_address_addr_space_p (mem_mode, addr, as)) @@ -3262,7 +3264,7 @@ get_address_cost (bool symbol_present, bool var_present, data->max_offset = i == start ? 0 : i >> 1; off = data->max_offset; - for (i = start; i <= 1 << 20; i <<= 1) + for (i = start; i && i <= (HOST_WIDE_INT) 1 << width; i <<= 1) { XEXP (addr, 1) = gen_int_mode (-i, address_mode); if (!memory_address_addr_space_p (mem_mode, addr, as)) @@ -3273,12 +3275,12 @@ get_address_cost (bool symbol_present, bool var_present, 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;