> -----Original Message----- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of bin.cheng > Sent: Friday, September 27, 2013 1:07 PM > To: 'Richard Biener' > Cc: GCC Patches > Subject: RE: [PATCH]Fix computation of offset in ivopt > > > > > -----Original Message----- > > From: Richard Biener [mailto:richard.guent...@gmail.com] > > Sent: Tuesday, September 24, 2013 6:31 PM > > To: Bin Cheng > > Cc: GCC Patches > > Subject: Re: [PATCH]Fix computation of offset in ivopt > > > > On Tue, Sep 24, 2013 at 11:13 AM, bin.cheng <bin.ch...@arm.com> wrote: > > > > + field = TREE_OPERAND (expr, 1); > > + if (DECL_FIELD_BIT_OFFSET (field) > > + && cst_and_fits_in_hwi (DECL_FIELD_BIT_OFFSET (field))) > > + boffset = int_cst_value (DECL_FIELD_BIT_OFFSET (field)); > > + > > + tmp = component_ref_field_offset (expr); > > + if (top_compref > > + && cst_and_fits_in_hwi (tmp)) > > + { > > + /* Strip the component reference completely. */ > > + op0 = TREE_OPERAND (expr, 0); > > + op0 = strip_offset_1 (op0, inside_addr, top_compref, &off0); > > + *offset = off0 + int_cst_value (tmp) + boffset / > BITS_PER_UNIT; > > + return op0; > > + } > > > > the failure paths seem mangled, that is, if cst_and_fits_in_hwi is > > false > for > > either offset part you may end up doing half accounting and not stripping. > > > > Btw, DECL_FIELD_BIT_OFFSET is always non-NULL. I suggest to rewrite > > to > > > > if (!inside_addr) > > return orig_expr; > > > > tmp = component_ref_field_offset (expr); > > field = TREE_OPERAND (expr, 1); > > if (top_compref > > && cst_and_fits_in_hwi (tmp) > > && cst_and_fits_in_hwi (DECL_FIELD_BIT_OFFSET (field))) > > { > > ... > > } > Will be refined. > > > > > note that this doesn't really handle overflows correctly as > > > > + *offset = off0 + int_cst_value (tmp) + boffset / > > + BITS_PER_UNIT; > > > > may still overflow. > Since it's "unsigned + signed + signed", according to implicit conversion, the > signed operand will be converted to unsigned, so the overflow would only > happen when off0 is huge number and tmp/boffset is large positive number, > right? Do I need to check whether off0 is larger than the overflowed result? > Also there is signed->unsigned problem here, see below. > > > > > @@ -4133,6 +4142,9 @@ get_computation_cost_at (struct ivopts_data > *data, > > bitmap_clear (*depends_on); > > } > > > > + /* Sign-extend offset if utype has lower precision than > > + HOST_WIDE_INT. */ offset = sext_hwi (offset, TYPE_PRECISION > > + (utype)); > > + > > > > offset is computed elsewhere in difference_cost and the bug to me > > seems that it is unsigned. sign-extending it here is odd at least > > (and the > extension > > should probably happen at sizetype precision, not that of utype). > I agree, The root cause is in split_offset_1, in which offset is computed. > Every time offset is computed in this function with a signed operand (like > "int_cst_value (tmp)" above), we need to take care the possible negative > number problem. Take this case as an example, we need to do below > change: > > case INTEGER_CST: > //....... > *offset = int_cst_value (expr); > change to > case INTEGER_CST: > //....... > *offset = sext_hwi (int_cst_value (expr), type); > > and > case MULT_EXPR: > //....... > *offset = sext_hwi (int_cst_value (expr), type); to > case MULT_EXPR: > //....... > HOST_WIDE_INT xxx = (HOST_WIDE_INT)off0 * int_cst_value (op1); > *offset = sext_hwi (xxx, type); > > Any comments?
Thought twice, I guess we can compute signed offset in strip_offset_1 and sign extend it for strip_offset, thus we don't need to change every computation of offset in that function. Thanks. bin