| Message ID | 87mwlfgp8s.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On Nov 8, 2013, at 2:30 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > From gcse.c: > > --- wide-int-base/gcc/gcc/gcse.c 2013-11-05 13:09:32.148376180 +0000 > +++ wide-int/gcc/gcc/gcse.c 2013-11-05 13:07:28.431495118 +0000 > @@ -1997,6 +1997,13 @@ prune_insertions_deletions (int n_elems) > bitmap_clear_bit (pre_delete_map[i], j); > } > > + if (dump_file) > + { > + dump_bitmap_vector (dump_file, "pre_insert_map", "", pre_insert_map, n_edges); > + dump_bitmap_vector (dump_file, "pre_delete_map", "", pre_delete_map, > + last_basic_block); > + } > + > sbitmap_free (prune_exprs); > free (insertions); > free (deletions); > > This doesn't look related. I think this was so Kenny could find code-gen differences and pin point them faster by having the ability to compare dump files. When we developed the branch, we found great utility in dump file comparisons. It narrows down the scope of what went wrong and helps get you into the right data structures to look at. Sometimes one can have identical code-gen, but, have different dump files, and comparing them and looking for differences was nice.
On 11/08/2013 05:30 AM, Richard Sandiford wrote: > Some comments from looking through the diff with the merge point, > ignoring wide-int.h and wide-int.cc. A few more to follow in the > form of patchses. > > -------------------- > > dwarf2out.c has: > > + case CONST_WIDE_INT: > + if (mode == VOIDmode) > + mode = GET_MODE (rtl); > + > + if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict)) > + { > + gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE (rtl)); > + > + /* Note that a CONST_DOUBLE rtx could represent either an integer > + or a floating-point constant. A CONST_DOUBLE is used whenever > + the constant requires more than one word in order to be > + adequately represented. We output CONST_DOUBLEs as blocks. */ > + loc_result = new_loc_descr (DW_OP_implicit_value, > + GET_MODE_SIZE (mode), 0); > + loc_result->dw_loc_oprnd2.val_class = dw_val_class_wide_int; > + loc_result->dw_loc_oprnd2.v.val_wide = ggc_alloc_cleared_wide_int (); > + *loc_result->dw_loc_oprnd2.v.val_wide = std::make_pair (rtl, mode); > > The comment looks like a cut-&-paste. The "mode == GET_MODE (rtl)" > bit should never be true. you are correct. > -------------------- > > From fold-const.c: > > @@ -13686,14 +13548,17 @@ fold_binary_loc (location_t loc, > break; > } > > - else if (TREE_INT_CST_HIGH (arg1) == signed_max_hi > - && TREE_INT_CST_LOW (arg1) == signed_max_lo > + else if (wi::eq_p (arg1, signed_max) > && TYPE_UNSIGNED (arg1_type) > + /* KENNY QUESTIONS THE CHECKING OF THE BITSIZE > + HERE. HE FEELS THAT THE PRECISION SHOULD BE > + CHECKED */ > + > /* We will flip the signedness of the comparison operator > associated with the mode of arg1, so the sign bit is > specified by this mode. Check that arg1 is the signed > max associated with this sign bit. */ > - && width == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) > + && prec == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) > /* signed_type does not work on pointer types. */ > && INTEGRAL_TYPE_P (arg1_type)) > { > > Looks like it should be resolved one way or the other before the merge. i am the one who asked the question. > > -------------------- > > From gcse.c: > > --- wide-int-base/gcc/gcc/gcse.c 2013-11-05 13:09:32.148376180 +0000 > +++ wide-int/gcc/gcc/gcse.c 2013-11-05 13:07:28.431495118 +0000 > @@ -1997,6 +1997,13 @@ prune_insertions_deletions (int n_elems) > bitmap_clear_bit (pre_delete_map[i], j); > } > > + if (dump_file) > + { > + dump_bitmap_vector (dump_file, "pre_insert_map", "", pre_insert_map, n_edges); > + dump_bitmap_vector (dump_file, "pre_delete_map", "", pre_delete_map, > + last_basic_block); > + } > + > sbitmap_free (prune_exprs); > free (insertions); > free (deletions); > > This doesn't look related. when we were a/b tests with trunk we added this to trunk also. This this is useful, but it could also be removed. > -------------------- > > From lcm.c: > > diff -udpr '--exclude=.svn' '--exclude=.pc' '--exclude=patches' wide-int-base/gcc/gcc/lcm.c wide-int/gcc/gcc/lcm.c > --- wide-int-base/gcc/gcc/lcm.c 2013-08-22 09:00:23.068716382 +0100 > +++ wide-int/gcc/gcc/lcm.c 2013-10-26 13:19:16.287277520 +0100 > @@ -64,6 +64,7 @@ along with GCC; see the file COPYING3. > #include "sbitmap.h" > #include "dumpfile.h" > > +#define LCM_DEBUG_INFO 1 > /* Edge based LCM routines. */ > static void compute_antinout_edge (sbitmap *, sbitmap *, sbitmap *, sbitmap *); > static void compute_earliest (struct edge_list *, int, sbitmap *, sbitmap *, > @@ -106,6 +107,7 @@ compute_antinout_edge (sbitmap *antloc, > /* We want a maximal solution, so make an optimistic initialization of > ANTIN. */ > bitmap_vector_ones (antin, last_basic_block); > + bitmap_vector_clear (antout, last_basic_block); > > /* Put every block on the worklist; this is necessary because of the > optimistic initialization of ANTIN above. */ > @@ -432,6 +434,7 @@ pre_edge_lcm (int n_exprs, sbitmap *tran > > /* Allocate an extra element for the exit block in the laterin vector. */ > laterin = sbitmap_vector_alloc (last_basic_block + 1, n_exprs); > + bitmap_vector_clear (laterin, last_basic_block); > compute_laterin (edge_list, earliest, antloc, later, laterin); > > #ifdef LCM_DEBUG_INFO this is less so. it was added for debugging. But the problem was that the bitvectors were never initialized. it could be argued that in correct code that this would not matter unless you actually were looking the values for debugging which we were doing. I would certainly say that you could remove the define, but others should comment on removing the clears. > Same here. > > -------------------- > > From real.c: > > @@ -2144,43 +2148,131 @@ real_from_string3 (REAL_VALUE_TYPE *r, c > real_convert (r, mode, r); > } > > -/* Initialize R from the integer pair HIGH+LOW. */ > +/* Initialize R from a HOST_WIDE_INT. */ > > void > real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, > - unsigned HOST_WIDE_INT low, HOST_WIDE_INT high, > - int unsigned_p) > + HOST_WIDE_INT val, > + signop sgn) > { > - if (low == 0 && high == 0) > + if (val == 0) > get_zero (r, 0); > else > { > memset (r, 0, sizeof (*r)); > r->cl = rvc_normal; > - r->sign = high < 0 && !unsigned_p; > - SET_REAL_EXP (r, HOST_BITS_PER_DOUBLE_INT); > + r->sign = val < 0 && sgn == SIGNED; > + SET_REAL_EXP (r, HOST_BITS_PER_WIDE_INT); > > + /* TODO: This fails for -MAXHOSTWIDEINT, wide_int version would > + have worked. */ > if (r->sign) > + val = -val; > > Given the TODO, is it really worth having this HOST_WIDE_INT version? > Wouldn't it be better to have just the wide_int version? sounds like a good idea to me. > -------------------- > > From real.c: > > +/* Initialize R from the integer pair HIGH+LOW. */ > + > +void > +real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, > + const wide_int_ref &val_in, signop sgn) > +{ > + if (val_in == 0) > + get_zero (r, 0); > + else > + { > + unsigned int len = val_in.get_precision (); > + int i, j, e=0; > + int maxbitlen = MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT; > > Why + HOST_BITS_PER_WIDE_INT? AIUI you want + X so that you can negate > the largest negative number, but 1 bit should be enough for that. yes i was just looking for 1 extra bit. > In case the idea was to get a rounded bitlen, there's no guarantee that > MAX_BITSIZE_MODE_ANY_INT itself is rounded. > > + HOST_WIDE_INT cnt_l_z; > + cnt_l_z = wi::clz (val); > + > + if (maxbitlen - cnt_l_z > realmax) > + { > + e = maxbitlen - cnt_l_z - realmax; > + > + /* This value is too large, we must shift it right to > + preserve all the bits we can, and then bump the > + exponent up by that amount. */ > + val = wi::lrshift (val, e); > > Don't we want a sticky bit for the shifted-out bits, in order to detect > the 0.5 ULP case properly? i do not know. you could be correct. > -------------------- > > From recog.c: > > +int > +const_scalar_int_operand (rtx op, enum machine_mode mode) > +{ > + if (!CONST_SCALAR_INT_P (op)) > + return 0; > + > + if (CONST_INT_P (op)) > + return const_int_operand (op, mode); > + > + if (mode != VOIDmode) > + { > + int prec = GET_MODE_PRECISION (mode); > + int bitsize = GET_MODE_BITSIZE (mode); > + > + if (CONST_WIDE_INT_NUNITS (op) * HOST_BITS_PER_WIDE_INT > bitsize) > + return 0; > + > + if (prec == bitsize) > + return 1; > + else > + { > + /* Multiword partial int. */ > + HOST_WIDE_INT x > + = CONST_WIDE_INT_ELT (op, CONST_WIDE_INT_NUNITS (op) - 1); > + return (sext_hwi (x, prec & (HOST_BITS_PER_WIDE_INT - 1)) == x); > + } > + } > + return 1; > +} > > const_int_operand rejects cases where gen_int_mode (INTVAL (op), mode) > would give something different from op. We need to do the equivalent > for CONST_WIDE_INT too. (The current CONST_DOUBLE code doesn't bother > because at the moment the only >HWI mode we support is a 2*HWI one.) > > It'd be nice to handle CONST_INT and CONST_WIDE_INT using the same code, > without going to const_int_operand. > > +/* Returns 1 if OP is an operand that is a CONST_WIDE_INT of mode > + MODE. This most likely is not as useful as > + const_scalar_int_operand since it does not accept CONST_INTs, but > + is here for consistancy. */ > +int > +const_wide_int_operand (rtx op, enum machine_mode mode) > +{ > + if (!CONST_WIDE_INT_P (op)) > + return 0; > + > + return const_scalar_int_operand (op, mode); > +} > > Hmm, but have you found any use cases? Like you say in the comment, > it just seems wrong to require a CONST_WIDE_INT over a CONST_INT, > since that's a host-dependent choice. I think we should drop this > from the initial merge. no, but i have only converted the ppc to use this so far. however i am skeptical that there will be any clients. it could die and i would not be unhappy. > -------------------- > > From rtl.c: > > +/* Write the wide constant X to OUTFILE. */ > + > +void > +cwi_output_hex (FILE *outfile, const_rtx x) > +{ > + int i = CWI_GET_NUM_ELEM (x); > + gcc_assert (i > 0); > + if (CWI_ELT (x, i-1) == 0) > + fprintf (outfile, "0x"); > + fprintf (outfile, HOST_WIDE_INT_PRINT_HEX, CWI_ELT (x, --i)); > + while (--i >= 0) > + fprintf (outfile, HOST_WIDE_INT_PRINT_PADDED_HEX, CWI_ELT (x, i)); > +} > > Why only print "0x" if the top element is 0? Probably deserves a comment. I will add a comment. > -------------------- > > From tree-cfg.c: > > @@ -2698,24 +2700,25 @@ verify_expr (tree *tp, int *walk_subtree > > if (TREE_CODE (t) == BIT_FIELD_REF) > { > - if (!host_integerp (TREE_OPERAND (t, 1), 1) > - || !host_integerp (TREE_OPERAND (t, 2), 1)) > + if (!tree_fits_uhwi_p (TREE_OPERAND (t, 1)) > + || !tree_fits_uhwi_p (TREE_OPERAND (t, 2))) > { > error ("invalid position or size operand to BIT_FIELD_REF"); > return t; > } > if (INTEGRAL_TYPE_P (TREE_TYPE (t)) > && (TYPE_PRECISION (TREE_TYPE (t)) > - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) > + != tree_to_uhwi (TREE_OPERAND (t, 1)))) > { > error ("integral result type precision does not match " > "field size of BIT_FIELD_REF"); > return t; > } > else if (!INTEGRAL_TYPE_P (TREE_TYPE (t)) > + && !AGGREGATE_TYPE_P (TREE_TYPE (t)) > && TYPE_MODE (TREE_TYPE (t)) != BLKmode > && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t))) > - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) > + != tree_to_uhwi (TREE_OPERAND (t, 1)))) > { > error ("mode precision of non-integral result does not " > "match field size of BIT_FIELD_REF"); > > Why the extra !AGGREGATE_TYPE_P test? looks like a typo to me. > > -------------------- > > From tree-vrp.c: > > @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code > /* If the singed operation wraps then int_const_binop has done > everything we want. */ > ; > + /* Signed division of -1/0 overflows and by the time it gets here > + returns NULL_TREE. */ > + else if (!res) > + return NULL_TREE; > else if ((TREE_OVERFLOW (res) > && !TREE_OVERFLOW (val1) > && !TREE_OVERFLOW (val2)) > > Why is this case different from trunk? Or is it a bug-fix independent > of wide-int? the api for division is different for wide-int than it was for double-int. > Thanks, > Richard
On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck <zadeck@naturalbridge.com> wrote: > On 11/08/2013 05:30 AM, Richard Sandiford wrote: >> From tree-vrp.c: >> >> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >> /* If the singed operation wraps then int_const_binop has done >> everything we want. */ >> ; >> + /* Signed division of -1/0 overflows and by the time it gets here >> + returns NULL_TREE. */ >> + else if (!res) >> + return NULL_TREE; >> else if ((TREE_OVERFLOW (res) >> && !TREE_OVERFLOW (val1) >> && !TREE_OVERFLOW (val2)) >> >> Why is this case different from trunk? Or is it a bug-fix independent >> of wide-int? > > the api for division is different for wide-int than it was for double-int. But this is using a tree API (int_const_binop) that didn't change (it returned NULL for / 0 previously). So what makes us arrive here now? (I agree there is a bug in VRP, but it shouldn't manifest itself only on wide-int) Richard. >> >> Thanks, >> Richard > >
On 11/11/2013 06:49 AM, Richard Biener wrote: > On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck <zadeck@naturalbridge.com> wrote: >> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>> From tree-vrp.c: >>> >>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>> /* If the singed operation wraps then int_const_binop has done >>> everything we want. */ >>> ; >>> + /* Signed division of -1/0 overflows and by the time it gets here >>> + returns NULL_TREE. */ >>> + else if (!res) >>> + return NULL_TREE; >>> else if ((TREE_OVERFLOW (res) >>> && !TREE_OVERFLOW (val1) >>> && !TREE_OVERFLOW (val2)) >>> >>> Why is this case different from trunk? Or is it a bug-fix independent >>> of wide-int? >> the api for division is different for wide-int than it was for double-int. > But this is using a tree API (int_const_binop) that didn't change > (it returned NULL for / 0 previously). So what makes us arrive here > now? (I agree there is a bug in VRP, but it shouldn't manifest itself > only on wide-int) > > Richard. My reading of the code is that is that i changed int_const_binop to return null_tree for this case. On the trunk, only rem returns null_tree for divide by 0, on the wide int branch, both div and rem return null tree. I know that this is going to bring on a string of questions that i do not remember the answers to as to why i made that change. but fold-const.c:int_const_binop_1 now returns null_tree and this is just fallout from that change. Kenny >>> Thanks, >>> Richard >>
On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck <zadeck@naturalbridge.com> wrote: > On 11/11/2013 06:49 AM, Richard Biener wrote: >> >> On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck <zadeck@naturalbridge.com> >> wrote: >>> >>> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>>> >>>> From tree-vrp.c: >>>> >>>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>>> /* If the singed operation wraps then int_const_binop has done >>>> everything we want. */ >>>> ; >>>> + /* Signed division of -1/0 overflows and by the time it gets here >>>> + returns NULL_TREE. */ >>>> + else if (!res) >>>> + return NULL_TREE; >>>> else if ((TREE_OVERFLOW (res) >>>> && !TREE_OVERFLOW (val1) >>>> && !TREE_OVERFLOW (val2)) >>>> >>>> Why is this case different from trunk? Or is it a bug-fix independent >>>> of wide-int? >>> >>> the api for division is different for wide-int than it was for >>> double-int. >> >> But this is using a tree API (int_const_binop) that didn't change >> (it returned NULL for / 0 previously). So what makes us arrive here >> now? (I agree there is a bug in VRP, but it shouldn't manifest itself >> only on wide-int) >> >> Richard. > > My reading of the code is that is that i changed int_const_binop to return > null_tree for this case. Trunk has: case TRUNC_DIV_EXPR: case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR: case EXACT_DIV_EXPR: /* This is a shortcut for a common special case. */ if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0 && !TREE_OVERFLOW (arg1) && !TREE_OVERFLOW (arg2) && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0) { if (code == CEIL_DIV_EXPR) op1.low += op2.low - 1; res.low = op1.low / op2.low, res.high = 0; break; } /* ... fall through ... */ case ROUND_DIV_EXPR: if (op2.is_zero ()) return NULL_TREE; so it already returns NULL_TREE on divide by zero. > On the trunk, only rem returns null_tree for divide by 0, on the wide int > branch, both div and rem return null tree. > > I know that this is going to bring on a string of questions that i do not > remember the answers to as to why i made that change. but > fold-const.c:int_const_binop_1 now returns null_tree and this is just > fallout from that change. > > Kenny >>>> >>>> Thanks, >>>> Richard >>> >>> >
On 11/11/2013 09:42 AM, Richard Biener wrote: > On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck > <zadeck@naturalbridge.com> wrote: >> On 11/11/2013 06:49 AM, Richard Biener wrote: >>> On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck <zadeck@naturalbridge.com> >>> wrote: >>>> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>>>> From tree-vrp.c: >>>>> >>>>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>>>> /* If the singed operation wraps then int_const_binop has done >>>>> everything we want. */ >>>>> ; >>>>> + /* Signed division of -1/0 overflows and by the time it gets here >>>>> + returns NULL_TREE. */ >>>>> + else if (!res) >>>>> + return NULL_TREE; >>>>> else if ((TREE_OVERFLOW (res) >>>>> && !TREE_OVERFLOW (val1) >>>>> && !TREE_OVERFLOW (val2)) >>>>> >>>>> Why is this case different from trunk? Or is it a bug-fix independent >>>>> of wide-int? >>>> the api for division is different for wide-int than it was for >>>> double-int. >>> But this is using a tree API (int_const_binop) that didn't change >>> (it returned NULL for / 0 previously). So what makes us arrive here >>> now? (I agree there is a bug in VRP, but it shouldn't manifest itself >>> only on wide-int) >>> >>> Richard. >> My reading of the code is that is that i changed int_const_binop to return >> null_tree for this case. > Trunk has: > > case TRUNC_DIV_EXPR: > case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR: > case EXACT_DIV_EXPR: > /* This is a shortcut for a common special case. */ > if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0 > && !TREE_OVERFLOW (arg1) > && !TREE_OVERFLOW (arg2) > && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0) > { > if (code == CEIL_DIV_EXPR) > op1.low += op2.low - 1; > > res.low = op1.low / op2.low, res.high = 0; > break; > } > > /* ... fall through ... */ > > case ROUND_DIV_EXPR: > if (op2.is_zero ()) > return NULL_TREE; > > so it already returns NULL_TREE on divide by zero. I found the reason!!!! This is one of the many "tree-vrp was not properly tested for TImode bugs." on the trunk, the case 0/(smallest negative number) case will only trigger overflow in TImode. On the wide-int branch, tree-vrp works at the precision of the operands so overflow is triggered properly for this case. So for HImode, the trunk produces the a result for 0/0x80 and then force_fit code at the bottom of int_const_binop_1 turns this into an overflow tree value rather than a null tree. on the wide-int branch, this case causes the overflow bit to be returned from the wide-int divide because the overflow case is properly handled for all types and that overflow is turned into null_tree by the wide-int version of int_const_binop_1. apparently there are no test cases that exercise the true divide by 0 case but there are test cases that hit the 0/ largest negative number case for modes smaller than TImode. Kenny > >> On the trunk, only rem returns null_tree for divide by 0, on the wide int >> branch, both div and rem return null tree. >> >> I know that this is going to bring on a string of questions that i do not >> remember the answers to as to why i made that change. but >> fold-const.c:int_const_binop_1 now returns null_tree and this is just >> fallout from that change. >> >> Kenny >>>>> Thanks, >>>>> Richard >>>>
On Mon, Nov 11, 2013 at 4:04 PM, Kenneth Zadeck <zadeck@naturalbridge.com> wrote: > On 11/11/2013 09:42 AM, Richard Biener wrote: >> >> On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck >> <zadeck@naturalbridge.com> wrote: >>> >>> On 11/11/2013 06:49 AM, Richard Biener wrote: >>>> >>>> On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck >>>> <zadeck@naturalbridge.com> >>>> wrote: >>>>> >>>>> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>>>>> >>>>>> From tree-vrp.c: >>>>>> >>>>>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>>>>> /* If the singed operation wraps then int_const_binop has done >>>>>> everything we want. */ >>>>>> ; >>>>>> + /* Signed division of -1/0 overflows and by the time it gets here >>>>>> + returns NULL_TREE. */ >>>>>> + else if (!res) >>>>>> + return NULL_TREE; >>>>>> else if ((TREE_OVERFLOW (res) >>>>>> && !TREE_OVERFLOW (val1) >>>>>> && !TREE_OVERFLOW (val2)) >>>>>> >>>>>> Why is this case different from trunk? Or is it a bug-fix independent >>>>>> of wide-int? >>>>> >>>>> the api for division is different for wide-int than it was for >>>>> double-int. >>>> >>>> But this is using a tree API (int_const_binop) that didn't change >>>> (it returned NULL for / 0 previously). So what makes us arrive here >>>> now? (I agree there is a bug in VRP, but it shouldn't manifest itself >>>> only on wide-int) >>>> >>>> Richard. >>> >>> My reading of the code is that is that i changed int_const_binop to >>> return >>> null_tree for this case. >> >> Trunk has: >> >> case TRUNC_DIV_EXPR: >> case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR: >> case EXACT_DIV_EXPR: >> /* This is a shortcut for a common special case. */ >> if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0 >> && !TREE_OVERFLOW (arg1) >> && !TREE_OVERFLOW (arg2) >> && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0) >> { >> if (code == CEIL_DIV_EXPR) >> op1.low += op2.low - 1; >> >> res.low = op1.low / op2.low, res.high = 0; >> break; >> } >> >> /* ... fall through ... */ >> >> case ROUND_DIV_EXPR: >> if (op2.is_zero ()) >> return NULL_TREE; >> >> so it already returns NULL_TREE on divide by zero. > > I found the reason!!!! This is one of the many "tree-vrp was not properly > tested for TImode bugs." > > on the trunk, the case 0/(smallest negative number) case will only trigger > overflow in TImode. On the wide-int branch, tree-vrp works at the > precision of the operands so overflow is triggered properly for this case. > So for HImode, the trunk produces the a result for 0/0x80 and then force_fit > code at the bottom of int_const_binop_1 turns this into an overflow tree > value rather than a null tree. > > on the wide-int branch, this case causes the overflow bit to be returned > from the wide-int divide because the overflow case is properly handled for > all types and that overflow is turned into null_tree by the wide-int version > of int_const_binop_1. > > apparently there are no test cases that exercise the true divide by 0 case > but there are test cases that hit the 0/ largest negative number case for > modes smaller than TImode. You probably mean <largest negative number> / -1? I don't see where NULL_TREE is returned for any overflow case in int_const_binop_1. Ah, you made it so. That looks like a bogus change. What's the reason? int_const_binop_1 is supposed to return a value with TREE_OVERFLOW set in these cases, also required for frontend constant folding. Try compiling const int i = (-__INT_MAX__ - 1) / -1; which should say > ./cc1 -quiet t.c t.c:1:34: warning: integer overflow in expression [-Woverflow] const int i = (-__INT_MAX__ - 1) / -1; ^ but not error or ICE. Seems to work on the branch, but only because the expression is still folded by 12357 /* X / -1 is -X. */ 12358 if (!TYPE_UNSIGNED (type) 12359 && TREE_CODE (arg1) == INTEGER_CST 12360 && wi::eq_p (arg1, -1)) 12361 return fold_convert_loc (loc, type, negate_expr (arg0)); Thus case TRUNC_DIV_EXPR: case EXACT_DIV_EXPR: res = wi::div_trunc (arg1, arg2, sign, &overflow); if (overflow) return NULL_TREE; break; should retain the arg2 == 0 checks (and return NULL_TREE) but otherwise keep overflow handling the same. Richard. > Kenny > >> >>> On the trunk, only rem returns null_tree for divide by 0, on the wide int >>> branch, both div and rem return null tree. >>> >>> I know that this is going to bring on a string of questions that i do not >>> remember the answers to as to why i made that change. but >>> fold-const.c:int_const_binop_1 now returns null_tree and this is just >>> fallout from that change. >>> >>> Kenny >>>>>> >>>>>> Thanks, >>>>>> Richard >>>>> >>>>> >
On 11/11/2013 10:04 AM, Kenneth Zadeck wrote: > On 11/11/2013 09:42 AM, Richard Biener wrote: >> On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck >> <zadeck@naturalbridge.com> wrote: >>> On 11/11/2013 06:49 AM, Richard Biener wrote: >>>> On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck >>>> <zadeck@naturalbridge.com> >>>> wrote: >>>>> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>>>>> From tree-vrp.c: >>>>>> >>>>>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>>>>> /* If the singed operation wraps then int_const_binop has >>>>>> done >>>>>> everything we want. */ >>>>>> ; >>>>>> + /* Signed division of -1/0 overflows and by the time it gets here >>>>>> + returns NULL_TREE. */ >>>>>> + else if (!res) >>>>>> + return NULL_TREE; >>>>>> else if ((TREE_OVERFLOW (res) >>>>>> && !TREE_OVERFLOW (val1) >>>>>> && !TREE_OVERFLOW (val2)) >>>>>> >>>>>> Why is this case different from trunk? Or is it a bug-fix >>>>>> independent >>>>>> of wide-int? >>>>> the api for division is different for wide-int than it was for >>>>> double-int. >>>> But this is using a tree API (int_const_binop) that didn't change >>>> (it returned NULL for / 0 previously). So what makes us arrive here >>>> now? (I agree there is a bug in VRP, but it shouldn't manifest itself >>>> only on wide-int) >>>> >>>> Richard. >>> My reading of the code is that is that i changed int_const_binop to >>> return >>> null_tree for this case. >> Trunk has: >> >> case TRUNC_DIV_EXPR: >> case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR: >> case EXACT_DIV_EXPR: >> /* This is a shortcut for a common special case. */ >> if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0 >> && !TREE_OVERFLOW (arg1) >> && !TREE_OVERFLOW (arg2) >> && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0) >> { >> if (code == CEIL_DIV_EXPR) >> op1.low += op2.low - 1; >> >> res.low = op1.low / op2.low, res.high = 0; >> break; >> } >> >> /* ... fall through ... */ >> >> case ROUND_DIV_EXPR: >> if (op2.is_zero ()) >> return NULL_TREE; >> >> so it already returns NULL_TREE on divide by zero. > I found the reason!!!! This is one of the many "tree-vrp was not > properly tested for TImode bugs." > it is an overstatement to say that this was a timode undertesting issue. I made the change to tree-vrp because of wide-int's treating of all divide/rem overflows in a uniform manner AND the fact that there was a untested divide by zero hole in tree-vrp. I most likely never saw a true divide by zero pass into that section of code. > on the trunk, the case 0/(smallest negative number) case will only > trigger overflow in TImode. On the wide-int branch, tree-vrp works > at the precision of the operands so overflow is triggered properly for > this case. So for HImode, the trunk produces the a result for > 0/0x80 and then force_fit code at the bottom of int_const_binop_1 > turns this into an overflow tree value rather than a null tree. > > on the wide-int branch, this case causes the overflow bit to be > returned from the wide-int divide because the overflow case is > properly handled for all types and that overflow is turned into > null_tree by the wide-int version of int_const_binop_1. > > apparently there are no test cases that exercise the true divide by 0 > case but there are test cases that hit the 0/ largest negative number > case for modes smaller than TImode. > > Kenny >> >>> On the trunk, only rem returns null_tree for divide by 0, on the >>> wide int >>> branch, both div and rem return null tree. >>> >>> I know that this is going to bring on a string of questions that i >>> do not >>> remember the answers to as to why i made that change. but >>> fold-const.c:int_const_binop_1 now returns null_tree and this is just >>> fallout from that change. >>> >>> Kenny >>>>>> Thanks, >>>>>> Richard >>>>> >
On 11/11/2013 10:29 AM, Richard Biener wrote: > On Mon, Nov 11, 2013 at 4:04 PM, Kenneth Zadeck > <zadeck@naturalbridge.com> wrote: >> On 11/11/2013 09:42 AM, Richard Biener wrote: >>> On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck >>> <zadeck@naturalbridge.com> wrote: >>>> On 11/11/2013 06:49 AM, Richard Biener wrote: >>>>> On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck >>>>> <zadeck@naturalbridge.com> >>>>> wrote: >>>>>> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>>>>>> From tree-vrp.c: >>>>>>> >>>>>>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>>>>>> /* If the singed operation wraps then int_const_binop has done >>>>>>> everything we want. */ >>>>>>> ; >>>>>>> + /* Signed division of -1/0 overflows and by the time it gets here >>>>>>> + returns NULL_TREE. */ >>>>>>> + else if (!res) >>>>>>> + return NULL_TREE; >>>>>>> else if ((TREE_OVERFLOW (res) >>>>>>> && !TREE_OVERFLOW (val1) >>>>>>> && !TREE_OVERFLOW (val2)) >>>>>>> >>>>>>> Why is this case different from trunk? Or is it a bug-fix independent >>>>>>> of wide-int? >>>>>> the api for division is different for wide-int than it was for >>>>>> double-int. >>>>> But this is using a tree API (int_const_binop) that didn't change >>>>> (it returned NULL for / 0 previously). So what makes us arrive here >>>>> now? (I agree there is a bug in VRP, but it shouldn't manifest itself >>>>> only on wide-int) >>>>> >>>>> Richard. >>>> My reading of the code is that is that i changed int_const_binop to >>>> return >>>> null_tree for this case. >>> Trunk has: >>> >>> case TRUNC_DIV_EXPR: >>> case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR: >>> case EXACT_DIV_EXPR: >>> /* This is a shortcut for a common special case. */ >>> if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0 >>> && !TREE_OVERFLOW (arg1) >>> && !TREE_OVERFLOW (arg2) >>> && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0) >>> { >>> if (code == CEIL_DIV_EXPR) >>> op1.low += op2.low - 1; >>> >>> res.low = op1.low / op2.low, res.high = 0; >>> break; >>> } >>> >>> /* ... fall through ... */ >>> >>> case ROUND_DIV_EXPR: >>> if (op2.is_zero ()) >>> return NULL_TREE; >>> >>> so it already returns NULL_TREE on divide by zero. >> I found the reason!!!! This is one of the many "tree-vrp was not properly >> tested for TImode bugs." >> >> on the trunk, the case 0/(smallest negative number) case will only trigger >> overflow in TImode. On the wide-int branch, tree-vrp works at the >> precision of the operands so overflow is triggered properly for this case. >> So for HImode, the trunk produces the a result for 0/0x80 and then force_fit >> code at the bottom of int_const_binop_1 turns this into an overflow tree >> value rather than a null tree. >> >> on the wide-int branch, this case causes the overflow bit to be returned >> from the wide-int divide because the overflow case is properly handled for >> all types and that overflow is turned into null_tree by the wide-int version >> of int_const_binop_1. >> >> apparently there are no test cases that exercise the true divide by 0 case >> but there are test cases that hit the 0/ largest negative number case for >> modes smaller than TImode. > You probably mean <largest negative number> / -1? I don't see where > NULL_TREE is returned for any overflow case in int_const_binop_1. > > Ah, you made it so. That looks like a bogus change. What's the > reason? int_const_binop_1 is supposed to return a value with > TREE_OVERFLOW set in these cases, also required for frontend > constant folding. Try compiling > > const int i = (-__INT_MAX__ - 1) / -1; > > which should say > >> ./cc1 -quiet t.c > t.c:1:34: warning: integer overflow in expression [-Woverflow] > const int i = (-__INT_MAX__ - 1) / -1; > ^ > > but not error or ICE. Seems to work on the branch, but only > because the expression is still folded by > > 12357 /* X / -1 is -X. */ > 12358 if (!TYPE_UNSIGNED (type) > 12359 && TREE_CODE (arg1) == INTEGER_CST > 12360 && wi::eq_p (arg1, -1)) > 12361 return fold_convert_loc (loc, type, negate_expr (arg0)); > > > Thus > > case TRUNC_DIV_EXPR: > case EXACT_DIV_EXPR: > res = wi::div_trunc (arg1, arg2, sign, &overflow); > if (overflow) > return NULL_TREE; > break; > > should retain the arg2 == 0 checks (and return NULL_TREE) but > otherwise keep overflow handling the same. I see your point. i did not think that it was necessary to distinguish the two types of behavior by division. i will make this fix today and test and install it. kenny > Richard. > >> Kenny >> >>>> On the trunk, only rem returns null_tree for divide by 0, on the wide int >>>> branch, both div and rem return null tree. >>>> >>>> I know that this is going to bring on a string of questions that i do not >>>> remember the answers to as to why i made that change. but >>>> fold-const.c:int_const_binop_1 now returns null_tree and this is just >>>> fallout from that change. >>>> >>>> Kenny >>>>>>> Thanks, >>>>>>> Richard >>>>>>
Richi, i am having a little trouble putting this back the way that you want. The issue is rem. what is supposed to happen for INT_MIN % -1? I would assume because i am failing the last case of gcc.dg/c90-const-expr-8.c that INT_MIN %-1 should not overflow even if INT_MIN / -1 does. however, this appears to be achieved by "accident" on the trunk see (fold-const.c:1053 and below) because there is code in the that is identified as being a special case for speed that selects out the DI and shorter mode cases but allows the TImode to be passed to the double int code. The double int code will then produce overflow for this case because it does not know if this is a div, a mod, or both. In short, is the TImode code wrong here and we never produce overflow for mod? or is there something magical in the standard about the shorter types? kenny On 11/11/2013 10:29 AM, Richard Biener wrote: > On Mon, Nov 11, 2013 at 4:04 PM, Kenneth Zadeck > <zadeck@naturalbridge.com> wrote: >> On 11/11/2013 09:42 AM, Richard Biener wrote: >>> On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck >>> <zadeck@naturalbridge.com> wrote: >>>> On 11/11/2013 06:49 AM, Richard Biener wrote: >>>>> On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck >>>>> <zadeck@naturalbridge.com> >>>>> wrote: >>>>>> On 11/08/2013 05:30 AM, Richard Sandiford wrote: >>>>>>> From tree-vrp.c: >>>>>>> >>>>>>> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >>>>>>> /* If the singed operation wraps then int_const_binop has done >>>>>>> everything we want. */ >>>>>>> ; >>>>>>> + /* Signed division of -1/0 overflows and by the time it gets here >>>>>>> + returns NULL_TREE. */ >>>>>>> + else if (!res) >>>>>>> + return NULL_TREE; >>>>>>> else if ((TREE_OVERFLOW (res) >>>>>>> && !TREE_OVERFLOW (val1) >>>>>>> && !TREE_OVERFLOW (val2)) >>>>>>> >>>>>>> Why is this case different from trunk? Or is it a bug-fix independent >>>>>>> of wide-int? >>>>>> the api for division is different for wide-int than it was for >>>>>> double-int. >>>>> But this is using a tree API (int_const_binop) that didn't change >>>>> (it returned NULL for / 0 previously). So what makes us arrive here >>>>> now? (I agree there is a bug in VRP, but it shouldn't manifest itself >>>>> only on wide-int) >>>>> >>>>> Richard. >>>> My reading of the code is that is that i changed int_const_binop to >>>> return >>>> null_tree for this case. >>> Trunk has: >>> >>> case TRUNC_DIV_EXPR: >>> case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR: >>> case EXACT_DIV_EXPR: >>> /* This is a shortcut for a common special case. */ >>> if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0 >>> && !TREE_OVERFLOW (arg1) >>> && !TREE_OVERFLOW (arg2) >>> && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0) >>> { >>> if (code == CEIL_DIV_EXPR) >>> op1.low += op2.low - 1; >>> >>> res.low = op1.low / op2.low, res.high = 0; >>> break; >>> } >>> >>> /* ... fall through ... */ >>> >>> case ROUND_DIV_EXPR: >>> if (op2.is_zero ()) >>> return NULL_TREE; >>> >>> so it already returns NULL_TREE on divide by zero. >> I found the reason!!!! This is one of the many "tree-vrp was not properly >> tested for TImode bugs." >> >> on the trunk, the case 0/(smallest negative number) case will only trigger >> overflow in TImode. On the wide-int branch, tree-vrp works at the >> precision of the operands so overflow is triggered properly for this case. >> So for HImode, the trunk produces the a result for 0/0x80 and then force_fit >> code at the bottom of int_const_binop_1 turns this into an overflow tree >> value rather than a null tree. >> >> on the wide-int branch, this case causes the overflow bit to be returned >> from the wide-int divide because the overflow case is properly handled for >> all types and that overflow is turned into null_tree by the wide-int version >> of int_const_binop_1. >> >> apparently there are no test cases that exercise the true divide by 0 case >> but there are test cases that hit the 0/ largest negative number case for >> modes smaller than TImode. > You probably mean <largest negative number> / -1? I don't see where > NULL_TREE is returned for any overflow case in int_const_binop_1. > > Ah, you made it so. That looks like a bogus change. What's the > reason? int_const_binop_1 is supposed to return a value with > TREE_OVERFLOW set in these cases, also required for frontend > constant folding. Try compiling > > const int i = (-__INT_MAX__ - 1) / -1; > > which should say > >> ./cc1 -quiet t.c > t.c:1:34: warning: integer overflow in expression [-Woverflow] > const int i = (-__INT_MAX__ - 1) / -1; > ^ > > but not error or ICE. Seems to work on the branch, but only > because the expression is still folded by > > 12357 /* X / -1 is -X. */ > 12358 if (!TYPE_UNSIGNED (type) > 12359 && TREE_CODE (arg1) == INTEGER_CST > 12360 && wi::eq_p (arg1, -1)) > 12361 return fold_convert_loc (loc, type, negate_expr (arg0)); > > > Thus > > case TRUNC_DIV_EXPR: > case EXACT_DIV_EXPR: > res = wi::div_trunc (arg1, arg2, sign, &overflow); > if (overflow) > return NULL_TREE; > break; > > should retain the arg2 == 0 checks (and return NULL_TREE) but > otherwise keep overflow handling the same. > > Richard. > >> Kenny >> >>>> On the trunk, only rem returns null_tree for divide by 0, on the wide int >>>> branch, both div and rem return null tree. >>>> >>>> I know that this is going to bring on a string of questions that i do not >>>> remember the answers to as to why i made that change. but >>>> fold-const.c:int_const_binop_1 now returns null_tree and this is just >>>> fallout from that change. >>>> >>>> Kenny >>>>>>> Thanks, >>>>>>> Richard >>>>>>
On Tue, 12 Nov 2013, Kenneth Zadeck wrote: > Richi, > > i am having a little trouble putting this back the way that you want. The > issue is rem. > what is supposed to happen for INT_MIN % -1? > > I would assume because i am failing the last case of gcc.dg/c90-const-expr-8.c > that INT_MIN %-1 should not overflow even if INT_MIN / -1 does. however, Given the conclusion in C11 that a%b should be considered undefined if a/b is not representable, I think it's reasonable to say INT_MIN % -1 *should* be considered to overflow (for all C standard versions) (and bug 30484 is only a bug for -fwrapv).
On 11/12/2013 11:27 AM, Joseph S. Myers wrote: > On Tue, 12 Nov 2013, Kenneth Zadeck wrote: > >> Richi, >> >> i am having a little trouble putting this back the way that you want. The >> issue is rem. >> what is supposed to happen for INT_MIN % -1? >> >> I would assume because i am failing the last case of gcc.dg/c90-const-expr-8.c >> that INT_MIN %-1 should not overflow even if INT_MIN / -1 does. however, > Given the conclusion in C11 that a%b should be considered undefined if a/b > is not representable, I think it's reasonable to say INT_MIN % -1 *should* > be considered to overflow (for all C standard versions) (and bug 30484 is > only a bug for -fwrapv). > however, my local question is what do we want the api to be int-const-binop-1? The existing behavior seems to be that at least for common modes this function silently returns 0 and it is up to the front ends to put their own spin on it. kenny
On Tue, Nov 12, 2013 at 5:43 PM, Kenneth Zadeck <zadeck@naturalbridge.com> wrote: > On 11/12/2013 11:27 AM, Joseph S. Myers wrote: >> >> On Tue, 12 Nov 2013, Kenneth Zadeck wrote: >> >>> Richi, >>> >>> i am having a little trouble putting this back the way that you want. >>> The >>> issue is rem. >>> what is supposed to happen for INT_MIN % -1? >>> >>> I would assume because i am failing the last case of >>> gcc.dg/c90-const-expr-8.c >>> that INT_MIN %-1 should not overflow even if INT_MIN / -1 does. >>> however, >> >> Given the conclusion in C11 that a%b should be considered undefined if a/b >> is not representable, I think it's reasonable to say INT_MIN % -1 *should* >> be considered to overflow (for all C standard versions) (and bug 30484 is >> only a bug for -fwrapv). >> > however, my local question is what do we want the api to be > int-const-binop-1? The existing behavior seems to be that at least for > common modes this function silently returns 0 and it is up to the front ends > to put their own spin on it. For wide-int you create 1:1 the behavior of current trunk (if a change of behavior in TImode is not tested in the testsuite then you can ignore that). Whatever change you do to semantics of functions you do separately from wide-int (preferably first on trunk, or at your choice after the wide-int merge). For this case in question I'd say a % -1 should return 0, but for INT_MIN % -1 that 0 should have TREE_OVERFLOW set (and thus you need to adjust that c90-const-expr-8.c testcase). Richard. > kenny
Kenneth Zadeck <zadeck@naturalbridge.com> writes: >> From fold-const.c: >> >> @@ -13686,14 +13548,17 @@ fold_binary_loc (location_t loc, >> break; >> } >> >> - else if (TREE_INT_CST_HIGH (arg1) == signed_max_hi >> - && TREE_INT_CST_LOW (arg1) == signed_max_lo >> + else if (wi::eq_p (arg1, signed_max) >> && TYPE_UNSIGNED (arg1_type) >> + /* KENNY QUESTIONS THE CHECKING OF THE BITSIZE >> + HERE. HE FEELS THAT THE PRECISION SHOULD BE >> + CHECKED */ >> + >> /* We will flip the signedness of the comparison operator >> associated with the mode of arg1, so the sign bit is >> specified by this mode. Check that arg1 is the signed >> max associated with this sign bit. */ >> - && width == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) >> + && prec == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) >> /* signed_type does not work on pointer types. */ >> && INTEGRAL_TYPE_P (arg1_type)) >> { >> >> Looks like it should be resolved one way or the other before the merge. > i am the one who asked the question. OK, but this sort of thing should be handled separately from the wide-int conversion, e.g. by a question to gcc@ or a patch to gcc-patches@ that adds a "??? ..."-style comment. >> From gcse.c: >> >> --- wide-int-base/gcc/gcc/gcse.c 2013-11-05 13:09:32.148376180 +0000 >> +++ wide-int/gcc/gcc/gcse.c 2013-11-05 13:07:28.431495118 +0000 >> @@ -1997,6 +1997,13 @@ prune_insertions_deletions (int n_elems) >> bitmap_clear_bit (pre_delete_map[i], j); >> } >> >> + if (dump_file) >> + { >> + dump_bitmap_vector (dump_file, "pre_insert_map", "", pre_insert_map, n_edges); >> + dump_bitmap_vector (dump_file, "pre_delete_map", "", pre_delete_map, >> + last_basic_block); >> + } >> + >> sbitmap_free (prune_exprs); >> free (insertions); >> free (deletions); >> >> This doesn't look related. > when we were a/b tests with trunk we added this to trunk also. This this > is useful, but it could also be removed. > >> -------------------- >> >> From lcm.c: >> >> diff -udpr '--exclude=.svn' '--exclude=.pc' '--exclude=patches' wide-int-base/gcc/gcc/lcm.c wide-int/gcc/gcc/lcm.c >> --- wide-int-base/gcc/gcc/lcm.c 2013-08-22 09:00:23.068716382 +0100 >> +++ wide-int/gcc/gcc/lcm.c 2013-10-26 13:19:16.287277520 +0100 >> @@ -64,6 +64,7 @@ along with GCC; see the file COPYING3. >> #include "sbitmap.h" >> #include "dumpfile.h" >> >> +#define LCM_DEBUG_INFO 1 >> /* Edge based LCM routines. */ >> static void compute_antinout_edge (sbitmap *, sbitmap *, sbitmap *, sbitmap *); >> static void compute_earliest (struct edge_list *, int, sbitmap *, sbitmap *, >> @@ -106,6 +107,7 @@ compute_antinout_edge (sbitmap *antloc, >> /* We want a maximal solution, so make an optimistic initialization of >> ANTIN. */ >> bitmap_vector_ones (antin, last_basic_block); >> + bitmap_vector_clear (antout, last_basic_block); >> >> /* Put every block on the worklist; this is necessary because of the >> optimistic initialization of ANTIN above. */ >> @@ -432,6 +434,7 @@ pre_edge_lcm (int n_exprs, sbitmap *tran >> >> /* Allocate an extra element for the exit block in the laterin vector. */ >> laterin = sbitmap_vector_alloc (last_basic_block + 1, n_exprs); >> + bitmap_vector_clear (laterin, last_basic_block); >> compute_laterin (edge_list, earliest, antloc, later, laterin); >> >> #ifdef LCM_DEBUG_INFO > this is less so. it was added for debugging. But the problem was > that the bitvectors were never initialized. it could be argued that > in correct code that this would not matter unless you actually were > looking the values for debugging which we were doing. I would > certainly say that you could remove the define, but others should > comment on removing the clears. But here too I think this kind of change should be separate from the wide-int conversion. If you'd like to see some of these changes made then please submit a patch against mainline. >> +/* Returns 1 if OP is an operand that is a CONST_WIDE_INT of mode >> + MODE. This most likely is not as useful as >> + const_scalar_int_operand since it does not accept CONST_INTs, but >> + is here for consistancy. */ >> +int >> +const_wide_int_operand (rtx op, enum machine_mode mode) >> +{ >> + if (!CONST_WIDE_INT_P (op)) >> + return 0; >> + >> + return const_scalar_int_operand (op, mode); >> +} >> >> Hmm, but have you found any use cases? Like you say in the comment, >> it just seems wrong to require a CONST_WIDE_INT over a CONST_INT, >> since that's a host-dependent choice. I think we should drop this >> from the initial merge. > no, but i have only converted the ppc to use this so far. however i > am skeptical that there will be any clients. it could die and i would > not be unhappy. OK, well, since it's dead code at the moment and since IMO it's a bug waiting to happen, I'm pretty strongly in favour of dropping it. >> -------------------- >> >> From rtl.c: >> >> +/* Write the wide constant X to OUTFILE. */ >> + >> +void >> +cwi_output_hex (FILE *outfile, const_rtx x) >> +{ >> + int i = CWI_GET_NUM_ELEM (x); >> + gcc_assert (i > 0); >> + if (CWI_ELT (x, i-1) == 0) >> + fprintf (outfile, "0x"); >> + fprintf (outfile, HOST_WIDE_INT_PRINT_HEX, CWI_ELT (x, --i)); >> + while (--i >= 0) >> + fprintf (outfile, HOST_WIDE_INT_PRINT_PADDED_HEX, CWI_ELT (x, i)); >> +} >> >> Why only print "0x" if the top element is 0? Probably deserves a comment. > I will add a comment. Thanks. Richard
On 11/08/2013 05:30 AM, Richard Sandiford wrote: > Some comments from looking through the diff with the merge point, > ignoring wide-int.h and wide-int.cc. A few more to follow in the > form of patchses. > > -------------------- > > dwarf2out.c has: > > + case CONST_WIDE_INT: > + if (mode == VOIDmode) > + mode = GET_MODE (rtl); > + > + if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict)) > + { > + gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE (rtl)); > + > + /* Note that a CONST_DOUBLE rtx could represent either an integer > + or a floating-point constant. A CONST_DOUBLE is used whenever > + the constant requires more than one word in order to be > + adequately represented. We output CONST_DOUBLEs as blocks. */ > + loc_result = new_loc_descr (DW_OP_implicit_value, > + GET_MODE_SIZE (mode), 0); > + loc_result->dw_loc_oprnd2.val_class = dw_val_class_wide_int; > + loc_result->dw_loc_oprnd2.v.val_wide = ggc_alloc_cleared_wide_int (); > + *loc_result->dw_loc_oprnd2.v.val_wide = std::make_pair (rtl, mode); > > The comment looks like a cut-&-paste. The "mode == GET_MODE (rtl)" > bit should never be true. i removed the assertion completely. it really is unnecessary. > -------------------- > > From fold-const.c: > > @@ -13686,14 +13548,17 @@ fold_binary_loc (location_t loc, > break; > } > > - else if (TREE_INT_CST_HIGH (arg1) == signed_max_hi > - && TREE_INT_CST_LOW (arg1) == signed_max_lo > + else if (wi::eq_p (arg1, signed_max) > && TYPE_UNSIGNED (arg1_type) > + /* KENNY QUESTIONS THE CHECKING OF THE BITSIZE > + HERE. HE FEELS THAT THE PRECISION SHOULD BE > + CHECKED */ > + > /* We will flip the signedness of the comparison operator > associated with the mode of arg1, so the sign bit is > specified by this mode. Check that arg1 is the signed > max associated with this sign bit. */ > - && width == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) > + && prec == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) > /* signed_type does not work on pointer types. */ > && INTEGRAL_TYPE_P (arg1_type)) > { i have asked this question on the public newsgroup. This patch is consistent with the patch that i posted for trunk. i will modify this when i get feedback from the other one. > Looks like it should be resolved one way or the other before the merge. > > -------------------- > > From gcse.c: > > --- wide-int-base/gcc/gcc/gcse.c 2013-11-05 13:09:32.148376180 +0000 > +++ wide-int/gcc/gcc/gcse.c 2013-11-05 13:07:28.431495118 +0000 > @@ -1997,6 +1997,13 @@ prune_insertions_deletions (int n_elems) > bitmap_clear_bit (pre_delete_map[i], j); > } > > + if (dump_file) > + { > + dump_bitmap_vector (dump_file, "pre_insert_map", "", pre_insert_map, n_edges); > + dump_bitmap_vector (dump_file, "pre_delete_map", "", pre_delete_map, > + last_basic_block); > + } > + > sbitmap_free (prune_exprs); > free (insertions); > free (deletions); > > This doesn't look related. removed. > > -------------------- > > From lcm.c: > > diff -udpr '--exclude=.svn' '--exclude=.pc' '--exclude=patches' wide-int-base/gcc/gcc/lcm.c wide-int/gcc/gcc/lcm.c > --- wide-int-base/gcc/gcc/lcm.c 2013-08-22 09:00:23.068716382 +0100 > +++ wide-int/gcc/gcc/lcm.c 2013-10-26 13:19:16.287277520 +0100 > @@ -64,6 +64,7 @@ along with GCC; see the file COPYING3. > #include "sbitmap.h" > #include "dumpfile.h" > > +#define LCM_DEBUG_INFO 1 > /* Edge based LCM routines. */ > static void compute_antinout_edge (sbitmap *, sbitmap *, sbitmap *, sbitmap *); > static void compute_earliest (struct edge_list *, int, sbitmap *, sbitmap *, > @@ -106,6 +107,7 @@ compute_antinout_edge (sbitmap *antloc, > /* We want a maximal solution, so make an optimistic initialization of > ANTIN. */ > bitmap_vector_ones (antin, last_basic_block); > + bitmap_vector_clear (antout, last_basic_block); > > /* Put every block on the worklist; this is necessary because of the > optimistic initialization of ANTIN above. */ > @@ -432,6 +434,7 @@ pre_edge_lcm (int n_exprs, sbitmap *tran > > /* Allocate an extra element for the exit block in the laterin vector. */ > laterin = sbitmap_vector_alloc (last_basic_block + 1, n_exprs); > + bitmap_vector_clear (laterin, last_basic_block); > compute_laterin (edge_list, earliest, antloc, later, laterin); > > #ifdef LCM_DEBUG_INFO > > Same here. i removed this also. this was done for the sanity of debugging. but i do not think that it is important enough for the trunk. I assume that the code was done this way because it is faster to do this than explicitly clear the structures. > > -------------------- > > From real.c: > > @@ -2144,43 +2148,131 @@ real_from_string3 (REAL_VALUE_TYPE *r, c > real_convert (r, mode, r); > } > > -/* Initialize R from the integer pair HIGH+LOW. */ > +/* Initialize R from a HOST_WIDE_INT. */ > > void > real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, > - unsigned HOST_WIDE_INT low, HOST_WIDE_INT high, > - int unsigned_p) > + HOST_WIDE_INT val, > + signop sgn) > { > - if (low == 0 && high == 0) > + if (val == 0) > get_zero (r, 0); > else > { > memset (r, 0, sizeof (*r)); > r->cl = rvc_normal; > - r->sign = high < 0 && !unsigned_p; > - SET_REAL_EXP (r, HOST_BITS_PER_DOUBLE_INT); > + r->sign = val < 0 && sgn == SIGNED; > + SET_REAL_EXP (r, HOST_BITS_PER_WIDE_INT); > > + /* TODO: This fails for -MAXHOSTWIDEINT, wide_int version would > + have worked. */ > if (r->sign) > + val = -val; > > Given the TODO, is it really worth having this HOST_WIDE_INT version? > Wouldn't it be better to have just the wide_int version? i got rid of this version. > -------------------- > > From real.c: > > +/* Initialize R from the integer pair HIGH+LOW. */ > + > +void > +real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, > + const wide_int_ref &val_in, signop sgn) > +{ > + if (val_in == 0) > + get_zero (r, 0); > + else > + { > + unsigned int len = val_in.get_precision (); > + int i, j, e=0; > + int maxbitlen = MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT; > > Why + HOST_BITS_PER_WIDE_INT? AIUI you want + X so that you can negate > the largest negative number, but 1 bit should be enough for that. wide int works faster with precs that are multiples of hwi. > In case the idea was to get a rounded bitlen, there's no guarantee that > MAX_BITSIZE_MODE_ANY_INT itself is rounded. > > + HOST_WIDE_INT cnt_l_z; > + cnt_l_z = wi::clz (val); > + > + if (maxbitlen - cnt_l_z > realmax) > + { > + e = maxbitlen - cnt_l_z - realmax; > + > + /* This value is too large, we must shift it right to > + preserve all the bits we can, and then bump the > + exponent up by that amount. */ > + val = wi::lrshift (val, e); > > Don't we want a sticky bit for the shifted-out bits, in order to detect > the 0.5 ULP case properly? this is something for an fp expert. what we do here is upwards compatible with what was there before. > > -------------------- > > From recog.c: > > +int > +const_scalar_int_operand (rtx op, enum machine_mode mode) > +{ > + if (!CONST_SCALAR_INT_P (op)) > + return 0; > + > + if (CONST_INT_P (op)) > + return const_int_operand (op, mode); > + > + if (mode != VOIDmode) > + { > + int prec = GET_MODE_PRECISION (mode); > + int bitsize = GET_MODE_BITSIZE (mode); > + > + if (CONST_WIDE_INT_NUNITS (op) * HOST_BITS_PER_WIDE_INT > bitsize) > + return 0; > + > + if (prec == bitsize) > + return 1; > + else > + { > + /* Multiword partial int. */ > + HOST_WIDE_INT x > + = CONST_WIDE_INT_ELT (op, CONST_WIDE_INT_NUNITS (op) - 1); > + return (sext_hwi (x, prec & (HOST_BITS_PER_WIDE_INT - 1)) == x); > + } > + } > + return 1; > +} > > const_int_operand rejects cases where gen_int_mode (INTVAL (op), mode) > would give something different from op. We need to do the equivalent > for CONST_WIDE_INT too. (The current CONST_DOUBLE code doesn't bother > because at the moment the only >HWI mode we support is a 2*HWI one.) > > It'd be nice to handle CONST_INT and CONST_WIDE_INT using the same code, > without going to const_int_operand. > > +/* Returns 1 if OP is an operand that is a CONST_WIDE_INT of mode > + MODE. This most likely is not as useful as > + const_scalar_int_operand since it does not accept CONST_INTs, but > + is here for consistancy. */ > +int > +const_wide_int_operand (rtx op, enum machine_mode mode) > +{ > + if (!CONST_WIDE_INT_P (op)) > + return 0; > + > + return const_scalar_int_operand (op, mode); > +} > > Hmm, but have you found any use cases? Like you say in the comment, > it just seems wrong to require a CONST_WIDE_INT over a CONST_INT, > since that's a host-dependent choice. I think we should drop this > from the initial merge. its gone. > -------------------- > > From rtl.c: > > +/* Write the wide constant X to OUTFILE. */ > + > +void > +cwi_output_hex (FILE *outfile, const_rtx x) > +{ > + int i = CWI_GET_NUM_ELEM (x); > + gcc_assert (i > 0); > + if (CWI_ELT (x, i-1) == 0) > + fprintf (outfile, "0x"); > + fprintf (outfile, HOST_WIDE_INT_PRINT_HEX, CWI_ELT (x, --i)); > + while (--i >= 0) > + fprintf (outfile, HOST_WIDE_INT_PRINT_PADDED_HEX, CWI_ELT (x, i)); > +} > > Why only print "0x" if the top element is 0? Probably deserves a comment. > > -------------------- > > From tree-cfg.c: > > @@ -2698,24 +2700,25 @@ verify_expr (tree *tp, int *walk_subtree > > if (TREE_CODE (t) == BIT_FIELD_REF) > { > - if (!host_integerp (TREE_OPERAND (t, 1), 1) > - || !host_integerp (TREE_OPERAND (t, 2), 1)) > + if (!tree_fits_uhwi_p (TREE_OPERAND (t, 1)) > + || !tree_fits_uhwi_p (TREE_OPERAND (t, 2))) > { > error ("invalid position or size operand to BIT_FIELD_REF"); > return t; > } > if (INTEGRAL_TYPE_P (TREE_TYPE (t)) > && (TYPE_PRECISION (TREE_TYPE (t)) > - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) > + != tree_to_uhwi (TREE_OPERAND (t, 1)))) > { > error ("integral result type precision does not match " > "field size of BIT_FIELD_REF"); > return t; > } > else if (!INTEGRAL_TYPE_P (TREE_TYPE (t)) > + && !AGGREGATE_TYPE_P (TREE_TYPE (t)) > && TYPE_MODE (TREE_TYPE (t)) != BLKmode > && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t))) > - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) > + != tree_to_uhwi (TREE_OPERAND (t, 1)))) > { > error ("mode precision of non-integral result does not " > "match field size of BIT_FIELD_REF"); > > Why the extra !AGGREGATE_TYPE_P test? > > -------------------- > > From tree-vrp.c: > > @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code > /* If the singed operation wraps then int_const_binop has done > everything we want. */ > ; > + /* Signed division of -1/0 overflows and by the time it gets here > + returns NULL_TREE. */ > + else if (!res) > + return NULL_TREE; > else if ((TREE_OVERFLOW (res) > && !TREE_OVERFLOW (val1) > && !TREE_OVERFLOW (val2)) > > Why is this case different from trunk? Or is it a bug-fix independent > of wide-int? this will be handled in a separate set of patches. I am working on it. > > Thanks, > Richard
Please ignore the email i just sent that looks like this. I accidentally pushed the send button. it is not ready yet. kenny On 11/15/2013 12:49 PM, Kenneth Zadeck wrote: > On 11/08/2013 05:30 AM, Richard Sandiford wrote: >> Some comments from looking through the diff with the merge point, >> ignoring wide-int.h and wide-int.cc. A few more to follow in the >> form of patchses. >> >> -------------------- >> >> dwarf2out.c has: >> >> + case CONST_WIDE_INT: >> + if (mode == VOIDmode) >> + mode = GET_MODE (rtl); >> + >> + if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict)) >> + { >> + gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE >> (rtl)); >> + >> + /* Note that a CONST_DOUBLE rtx could represent either an integer >> + or a floating-point constant. A CONST_DOUBLE is used whenever >> + the constant requires more than one word in order to be >> + adequately represented. We output CONST_DOUBLEs as >> blocks. */ >> + loc_result = new_loc_descr (DW_OP_implicit_value, >> + GET_MODE_SIZE (mode), 0); >> + loc_result->dw_loc_oprnd2.val_class = dw_val_class_wide_int; >> + loc_result->dw_loc_oprnd2.v.val_wide = >> ggc_alloc_cleared_wide_int (); >> + *loc_result->dw_loc_oprnd2.v.val_wide = std::make_pair (rtl, >> mode); >> >> The comment looks like a cut-&-paste. The "mode == GET_MODE (rtl)" >> bit should never be true. > i removed the assertion completely. it really is unnecessary. >> -------------------- >> >> From fold-const.c: >> >> @@ -13686,14 +13548,17 @@ fold_binary_loc (location_t loc, >> break; >> } >> - else if (TREE_INT_CST_HIGH (arg1) == signed_max_hi >> - && TREE_INT_CST_LOW (arg1) == signed_max_lo >> + else if (wi::eq_p (arg1, signed_max) >> && TYPE_UNSIGNED (arg1_type) >> + /* KENNY QUESTIONS THE CHECKING OF THE BITSIZE >> + HERE. HE FEELS THAT THE PRECISION SHOULD BE >> + CHECKED */ >> + >> /* We will flip the signedness of the comparison operator >> associated with the mode of arg1, so the sign bit is >> specified by this mode. Check that arg1 is the signed >> max associated with this sign bit. */ >> - && width == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) >> + && prec == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) >> /* signed_type does not work on pointer types. */ >> && INTEGRAL_TYPE_P (arg1_type)) >> { > i have asked this question on the public newsgroup. This patch is > consistent with the patch that i posted for trunk. i will modify > this when i get feedback from the other one. > >> Looks like it should be resolved one way or the other before the merge. >> >> -------------------- >> >> From gcse.c: >> >> --- wide-int-base/gcc/gcc/gcse.c 2013-11-05 13:09:32.148376180 +0000 >> +++ wide-int/gcc/gcc/gcse.c 2013-11-05 13:07:28.431495118 +0000 >> @@ -1997,6 +1997,13 @@ prune_insertions_deletions (int n_elems) >> bitmap_clear_bit (pre_delete_map[i], j); >> } >> + if (dump_file) >> + { >> + dump_bitmap_vector (dump_file, "pre_insert_map", "", >> pre_insert_map, n_edges); >> + dump_bitmap_vector (dump_file, "pre_delete_map", "", >> pre_delete_map, >> + last_basic_block); >> + } >> + >> sbitmap_free (prune_exprs); >> free (insertions); >> free (deletions); >> >> This doesn't look related. > removed. >> >> -------------------- >> >> From lcm.c: >> >> diff -udpr '--exclude=.svn' '--exclude=.pc' '--exclude=patches' >> wide-int-base/gcc/gcc/lcm.c wide-int/gcc/gcc/lcm.c >> --- wide-int-base/gcc/gcc/lcm.c 2013-08-22 09:00:23.068716382 +0100 >> +++ wide-int/gcc/gcc/lcm.c 2013-10-26 13:19:16.287277520 +0100 >> @@ -64,6 +64,7 @@ along with GCC; see the file COPYING3. >> #include "sbitmap.h" >> #include "dumpfile.h" >> +#define LCM_DEBUG_INFO 1 >> /* Edge based LCM routines. */ >> static void compute_antinout_edge (sbitmap *, sbitmap *, sbitmap *, >> sbitmap *); >> static void compute_earliest (struct edge_list *, int, sbitmap *, >> sbitmap *, >> @@ -106,6 +107,7 @@ compute_antinout_edge (sbitmap *antloc, >> /* We want a maximal solution, so make an optimistic >> initialization of >> ANTIN. */ >> bitmap_vector_ones (antin, last_basic_block); >> + bitmap_vector_clear (antout, last_basic_block); >> /* Put every block on the worklist; this is necessary because >> of the >> optimistic initialization of ANTIN above. */ >> @@ -432,6 +434,7 @@ pre_edge_lcm (int n_exprs, sbitmap *tran >> /* Allocate an extra element for the exit block in the laterin >> vector. */ >> laterin = sbitmap_vector_alloc (last_basic_block + 1, n_exprs); >> + bitmap_vector_clear (laterin, last_basic_block); >> compute_laterin (edge_list, earliest, antloc, later, laterin); >> #ifdef LCM_DEBUG_INFO >> >> Same here. > i removed this also. this was done for the sanity of debugging. but > i do not think that it is important enough for the trunk. I assume > that the code was done this way because it is faster to do this than > explicitly clear the structures. > >> >> -------------------- >> >> From real.c: >> >> @@ -2144,43 +2148,131 @@ real_from_string3 (REAL_VALUE_TYPE *r, c >> real_convert (r, mode, r); >> } >> -/* Initialize R from the integer pair HIGH+LOW. */ >> +/* Initialize R from a HOST_WIDE_INT. */ >> void >> real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, >> - unsigned HOST_WIDE_INT low, HOST_WIDE_INT high, >> - int unsigned_p) >> + HOST_WIDE_INT val, >> + signop sgn) >> { >> - if (low == 0 && high == 0) >> + if (val == 0) >> get_zero (r, 0); >> else >> { >> memset (r, 0, sizeof (*r)); >> r->cl = rvc_normal; >> - r->sign = high < 0 && !unsigned_p; >> - SET_REAL_EXP (r, HOST_BITS_PER_DOUBLE_INT); >> + r->sign = val < 0 && sgn == SIGNED; >> + SET_REAL_EXP (r, HOST_BITS_PER_WIDE_INT); >> + /* TODO: This fails for -MAXHOSTWIDEINT, wide_int version would >> + have worked. */ >> if (r->sign) >> + val = -val; >> >> Given the TODO, is it really worth having this HOST_WIDE_INT version? >> Wouldn't it be better to have just the wide_int version? > i got rid of this version. >> -------------------- >> >> From real.c: >> >> +/* Initialize R from the integer pair HIGH+LOW. */ >> + >> +void >> +real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, >> + const wide_int_ref &val_in, signop sgn) >> +{ >> + if (val_in == 0) >> + get_zero (r, 0); >> + else >> + { >> + unsigned int len = val_in.get_precision (); >> + int i, j, e=0; >> + int maxbitlen = MAX_BITSIZE_MODE_ANY_INT + >> HOST_BITS_PER_WIDE_INT; >> >> Why + HOST_BITS_PER_WIDE_INT? AIUI you want + X so that you can negate >> the largest negative number, but 1 bit should be enough for that. > wide int works faster with precs that are multiples of hwi. >> In case the idea was to get a rounded bitlen, there's no guarantee that >> MAX_BITSIZE_MODE_ANY_INT itself is rounded. >> >> + HOST_WIDE_INT cnt_l_z; >> + cnt_l_z = wi::clz (val); >> + >> + if (maxbitlen - cnt_l_z > realmax) >> + { >> + e = maxbitlen - cnt_l_z - realmax; >> + >> + /* This value is too large, we must shift it right to >> + preserve all the bits we can, and then bump the >> + exponent up by that amount. */ >> + val = wi::lrshift (val, e); >> >> Don't we want a sticky bit for the shifted-out bits, in order to detect >> the 0.5 ULP case properly? > this is something for an fp expert. what we do here is upwards > compatible with what was there before. >> >> -------------------- >> >> From recog.c: >> >> +int >> +const_scalar_int_operand (rtx op, enum machine_mode mode) >> +{ >> + if (!CONST_SCALAR_INT_P (op)) >> + return 0; >> + >> + if (CONST_INT_P (op)) >> + return const_int_operand (op, mode); >> + >> + if (mode != VOIDmode) >> + { >> + int prec = GET_MODE_PRECISION (mode); >> + int bitsize = GET_MODE_BITSIZE (mode); >> + >> + if (CONST_WIDE_INT_NUNITS (op) * HOST_BITS_PER_WIDE_INT > >> bitsize) >> + return 0; >> + >> + if (prec == bitsize) >> + return 1; >> + else >> + { >> + /* Multiword partial int. */ >> + HOST_WIDE_INT x >> + = CONST_WIDE_INT_ELT (op, CONST_WIDE_INT_NUNITS (op) - 1); >> + return (sext_hwi (x, prec & (HOST_BITS_PER_WIDE_INT - 1)) == x); >> + } >> + } >> + return 1; >> +} >> >> const_int_operand rejects cases where gen_int_mode (INTVAL (op), mode) >> would give something different from op. We need to do the equivalent >> for CONST_WIDE_INT too. (The current CONST_DOUBLE code doesn't bother >> because at the moment the only >HWI mode we support is a 2*HWI one.) >> >> It'd be nice to handle CONST_INT and CONST_WIDE_INT using the same code, >> without going to const_int_operand. >> >> +/* Returns 1 if OP is an operand that is a CONST_WIDE_INT of mode >> + MODE. This most likely is not as useful as >> + const_scalar_int_operand since it does not accept CONST_INTs, but >> + is here for consistancy. */ >> +int >> +const_wide_int_operand (rtx op, enum machine_mode mode) >> +{ >> + if (!CONST_WIDE_INT_P (op)) >> + return 0; >> + >> + return const_scalar_int_operand (op, mode); >> +} >> >> Hmm, but have you found any use cases? Like you say in the comment, >> it just seems wrong to require a CONST_WIDE_INT over a CONST_INT, >> since that's a host-dependent choice. I think we should drop this >> from the initial merge. > its gone. >> -------------------- >> >> From rtl.c: >> >> +/* Write the wide constant X to OUTFILE. */ >> + >> +void >> +cwi_output_hex (FILE *outfile, const_rtx x) >> +{ >> + int i = CWI_GET_NUM_ELEM (x); >> + gcc_assert (i > 0); >> + if (CWI_ELT (x, i-1) == 0) >> + fprintf (outfile, "0x"); >> + fprintf (outfile, HOST_WIDE_INT_PRINT_HEX, CWI_ELT (x, --i)); >> + while (--i >= 0) >> + fprintf (outfile, HOST_WIDE_INT_PRINT_PADDED_HEX, CWI_ELT (x, i)); >> +} >> >> Why only print "0x" if the top element is 0? Probably deserves a >> comment. >> >> -------------------- >> >> From tree-cfg.c: >> >> @@ -2698,24 +2700,25 @@ verify_expr (tree *tp, int *walk_subtree >> if (TREE_CODE (t) == BIT_FIELD_REF) >> { >> - if (!host_integerp (TREE_OPERAND (t, 1), 1) >> - || !host_integerp (TREE_OPERAND (t, 2), 1)) >> + if (!tree_fits_uhwi_p (TREE_OPERAND (t, 1)) >> + || !tree_fits_uhwi_p (TREE_OPERAND (t, 2))) >> { >> error ("invalid position or size operand to BIT_FIELD_REF"); >> return t; >> } >> if (INTEGRAL_TYPE_P (TREE_TYPE (t)) >> && (TYPE_PRECISION (TREE_TYPE (t)) >> - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) >> + != tree_to_uhwi (TREE_OPERAND (t, 1)))) >> { >> error ("integral result type precision does not match " >> "field size of BIT_FIELD_REF"); >> return t; >> } >> else if (!INTEGRAL_TYPE_P (TREE_TYPE (t)) >> + && !AGGREGATE_TYPE_P (TREE_TYPE (t)) >> && TYPE_MODE (TREE_TYPE (t)) != BLKmode >> && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t))) >> - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) >> + != tree_to_uhwi (TREE_OPERAND (t, 1)))) >> { >> error ("mode precision of non-integral result does not " >> "match field size of BIT_FIELD_REF"); >> >> Why the extra !AGGREGATE_TYPE_P test? >> >> -------------------- >> >> From tree-vrp.c: >> >> @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code >> /* If the singed operation wraps then int_const_binop has done >> everything we want. */ >> ; >> + /* Signed division of -1/0 overflows and by the time it gets here >> + returns NULL_TREE. */ >> + else if (!res) >> + return NULL_TREE; >> else if ((TREE_OVERFLOW (res) >> && !TREE_OVERFLOW (val1) >> && !TREE_OVERFLOW (val2)) >> >> Why is this case different from trunk? Or is it a bug-fix independent >> of wide-int? > this will be handled in a separate set of patches. I am working on it. >> >> Thanks, >> Richard >
--- wide-int-base/gcc/gcc/gcse.c 2013-11-05 13:09:32.148376180 +0000 +++ wide-int/gcc/gcc/gcse.c 2013-11-05 13:07:28.431495118 +0000 @@ -1997,6 +1997,13 @@ prune_insertions_deletions (int n_elems) bitmap_clear_bit (pre_delete_map[i], j); } + if (dump_file) + { + dump_bitmap_vector (dump_file, "pre_insert_map", "", pre_insert_map, n_edges); + dump_bitmap_vector (dump_file, "pre_delete_map", "", pre_delete_map, + last_basic_block); + } + sbitmap_free (prune_exprs); free (insertions); free (deletions); This doesn't look related. -------------------- From lcm.c: diff -udpr '--exclude=.svn' '--exclude=.pc' '--exclude=patches' wide-int-base/gcc/gcc/lcm.c wide-int/gcc/gcc/lcm.c --- wide-int-base/gcc/gcc/lcm.c 2013-08-22 09:00:23.068716382 +0100 +++ wide-int/gcc/gcc/lcm.c 2013-10-26 13:19:16.287277520 +0100 @@ -64,6 +64,7 @@ along with GCC; see the file COPYING3. #include "sbitmap.h" #include "dumpfile.h" +#define LCM_DEBUG_INFO 1 /* Edge based LCM routines. */ static void compute_antinout_edge (sbitmap *, sbitmap *, sbitmap *, sbitmap *); static void compute_earliest (struct edge_list *, int, sbitmap *, sbitmap *, @@ -106,6 +107,7 @@ compute_antinout_edge (sbitmap *antloc, /* We want a maximal solution, so make an optimistic initialization of ANTIN. */ bitmap_vector_ones (antin, last_basic_block); + bitmap_vector_clear (antout, last_basic_block); /* Put every block on the worklist; this is necessary because of the optimistic initialization of ANTIN above. */ @@ -432,6 +434,7 @@ pre_edge_lcm (int n_exprs, sbitmap *tran /* Allocate an extra element for the exit block in the laterin vector. */ laterin = sbitmap_vector_alloc (last_basic_block + 1, n_exprs); + bitmap_vector_clear (laterin, last_basic_block); compute_laterin (edge_list, earliest, antloc, later, laterin); #ifdef LCM_DEBUG_INFO
Some comments from looking through the diff with the merge point, ignoring wide-int.h and wide-int.cc. A few more to follow in the form of patchses. -------------------- dwarf2out.c has: + case CONST_WIDE_INT: + if (mode == VOIDmode) + mode = GET_MODE (rtl); + + if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict)) + { + gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE (rtl)); + + /* Note that a CONST_DOUBLE rtx could represent either an integer + or a floating-point constant. A CONST_DOUBLE is used whenever + the constant requires more than one word in order to be + adequately represented. We output CONST_DOUBLEs as blocks. */ + loc_result = new_loc_descr (DW_OP_implicit_value, + GET_MODE_SIZE (mode), 0); + loc_result->dw_loc_oprnd2.val_class = dw_val_class_wide_int; + loc_result->dw_loc_oprnd2.v.val_wide = ggc_alloc_cleared_wide_int (); + *loc_result->dw_loc_oprnd2.v.val_wide = std::make_pair (rtl, mode); The comment looks like a cut-&-paste. The "mode == GET_MODE (rtl)" bit should never be true. -------------------- From fold-const.c: @@ -13686,14 +13548,17 @@ fold_binary_loc (location_t loc, break; } - else if (TREE_INT_CST_HIGH (arg1) == signed_max_hi - && TREE_INT_CST_LOW (arg1) == signed_max_lo + else if (wi::eq_p (arg1, signed_max) && TYPE_UNSIGNED (arg1_type) + /* KENNY QUESTIONS THE CHECKING OF THE BITSIZE + HERE. HE FEELS THAT THE PRECISION SHOULD BE + CHECKED */ + /* We will flip the signedness of the comparison operator associated with the mode of arg1, so the sign bit is specified by this mode. Check that arg1 is the signed max associated with this sign bit. */ - && width == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) + && prec == GET_MODE_BITSIZE (TYPE_MODE (arg1_type)) /* signed_type does not work on pointer types. */ && INTEGRAL_TYPE_P (arg1_type)) { Looks like it should be resolved one way or the other before the merge. -------------------- From gcse.c: Same here. -------------------- From real.c: @@ -2144,43 +2148,131 @@ real_from_string3 (REAL_VALUE_TYPE *r, c real_convert (r, mode, r); } -/* Initialize R from the integer pair HIGH+LOW. */ +/* Initialize R from a HOST_WIDE_INT. */ void real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, - unsigned HOST_WIDE_INT low, HOST_WIDE_INT high, - int unsigned_p) + HOST_WIDE_INT val, + signop sgn) { - if (low == 0 && high == 0) + if (val == 0) get_zero (r, 0); else { memset (r, 0, sizeof (*r)); r->cl = rvc_normal; - r->sign = high < 0 && !unsigned_p; - SET_REAL_EXP (r, HOST_BITS_PER_DOUBLE_INT); + r->sign = val < 0 && sgn == SIGNED; + SET_REAL_EXP (r, HOST_BITS_PER_WIDE_INT); + /* TODO: This fails for -MAXHOSTWIDEINT, wide_int version would + have worked. */ if (r->sign) + val = -val; Given the TODO, is it really worth having this HOST_WIDE_INT version? Wouldn't it be better to have just the wide_int version? -------------------- From real.c: +/* Initialize R from the integer pair HIGH+LOW. */ + +void +real_from_integer (REAL_VALUE_TYPE *r, enum machine_mode mode, + const wide_int_ref &val_in, signop sgn) +{ + if (val_in == 0) + get_zero (r, 0); + else + { + unsigned int len = val_in.get_precision (); + int i, j, e=0; + int maxbitlen = MAX_BITSIZE_MODE_ANY_INT + HOST_BITS_PER_WIDE_INT; Why + HOST_BITS_PER_WIDE_INT? AIUI you want + X so that you can negate the largest negative number, but 1 bit should be enough for that. In case the idea was to get a rounded bitlen, there's no guarantee that MAX_BITSIZE_MODE_ANY_INT itself is rounded. + HOST_WIDE_INT cnt_l_z; + cnt_l_z = wi::clz (val); + + if (maxbitlen - cnt_l_z > realmax) + { + e = maxbitlen - cnt_l_z - realmax; + + /* This value is too large, we must shift it right to + preserve all the bits we can, and then bump the + exponent up by that amount. */ + val = wi::lrshift (val, e); Don't we want a sticky bit for the shifted-out bits, in order to detect the 0.5 ULP case properly? -------------------- From recog.c: +int +const_scalar_int_operand (rtx op, enum machine_mode mode) +{ + if (!CONST_SCALAR_INT_P (op)) + return 0; + + if (CONST_INT_P (op)) + return const_int_operand (op, mode); + + if (mode != VOIDmode) + { + int prec = GET_MODE_PRECISION (mode); + int bitsize = GET_MODE_BITSIZE (mode); + + if (CONST_WIDE_INT_NUNITS (op) * HOST_BITS_PER_WIDE_INT > bitsize) + return 0; + + if (prec == bitsize) + return 1; + else + { + /* Multiword partial int. */ + HOST_WIDE_INT x + = CONST_WIDE_INT_ELT (op, CONST_WIDE_INT_NUNITS (op) - 1); + return (sext_hwi (x, prec & (HOST_BITS_PER_WIDE_INT - 1)) == x); + } + } + return 1; +} const_int_operand rejects cases where gen_int_mode (INTVAL (op), mode) would give something different from op. We need to do the equivalent for CONST_WIDE_INT too. (The current CONST_DOUBLE code doesn't bother because at the moment the only >HWI mode we support is a 2*HWI one.) It'd be nice to handle CONST_INT and CONST_WIDE_INT using the same code, without going to const_int_operand. +/* Returns 1 if OP is an operand that is a CONST_WIDE_INT of mode + MODE. This most likely is not as useful as + const_scalar_int_operand since it does not accept CONST_INTs, but + is here for consistancy. */ +int +const_wide_int_operand (rtx op, enum machine_mode mode) +{ + if (!CONST_WIDE_INT_P (op)) + return 0; + + return const_scalar_int_operand (op, mode); +} Hmm, but have you found any use cases? Like you say in the comment, it just seems wrong to require a CONST_WIDE_INT over a CONST_INT, since that's a host-dependent choice. I think we should drop this from the initial merge. -------------------- From rtl.c: +/* Write the wide constant X to OUTFILE. */ + +void +cwi_output_hex (FILE *outfile, const_rtx x) +{ + int i = CWI_GET_NUM_ELEM (x); + gcc_assert (i > 0); + if (CWI_ELT (x, i-1) == 0) + fprintf (outfile, "0x"); + fprintf (outfile, HOST_WIDE_INT_PRINT_HEX, CWI_ELT (x, --i)); + while (--i >= 0) + fprintf (outfile, HOST_WIDE_INT_PRINT_PADDED_HEX, CWI_ELT (x, i)); +} Why only print "0x" if the top element is 0? Probably deserves a comment. -------------------- From tree-cfg.c: @@ -2698,24 +2700,25 @@ verify_expr (tree *tp, int *walk_subtree if (TREE_CODE (t) == BIT_FIELD_REF) { - if (!host_integerp (TREE_OPERAND (t, 1), 1) - || !host_integerp (TREE_OPERAND (t, 2), 1)) + if (!tree_fits_uhwi_p (TREE_OPERAND (t, 1)) + || !tree_fits_uhwi_p (TREE_OPERAND (t, 2))) { error ("invalid position or size operand to BIT_FIELD_REF"); return t; } if (INTEGRAL_TYPE_P (TREE_TYPE (t)) && (TYPE_PRECISION (TREE_TYPE (t)) - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) + != tree_to_uhwi (TREE_OPERAND (t, 1)))) { error ("integral result type precision does not match " "field size of BIT_FIELD_REF"); return t; } else if (!INTEGRAL_TYPE_P (TREE_TYPE (t)) + && !AGGREGATE_TYPE_P (TREE_TYPE (t)) && TYPE_MODE (TREE_TYPE (t)) != BLKmode && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t))) - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) + != tree_to_uhwi (TREE_OPERAND (t, 1)))) { error ("mode precision of non-integral result does not " "match field size of BIT_FIELD_REF"); Why the extra !AGGREGATE_TYPE_P test? -------------------- From tree-vrp.c: @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code /* If the singed operation wraps then int_const_binop has done everything we want. */ ; + /* Signed division of -1/0 overflows and by the time it gets here + returns NULL_TREE. */ + else if (!res) + return NULL_TREE; else if ((TREE_OVERFLOW (res) && !TREE_OVERFLOW (val1) && !TREE_OVERFLOW (val2)) Why is this case different from trunk? Or is it a bug-fix independent of wide-int? Thanks, Richard