| Message ID | CAFiYyc0gnMe7j+DgTeudnpH-zD22nK7Cuv4a2zj4Va-anApTBA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
>> + /* Sign-extend @1 to TYPE. */ >> + w1 = w1.from (w1, TYPE_PRECISION (type), SIGNED); >> >> not sure why you do always sign-extend. If the inner op is unsigned >> and we widen then that's certainly bogus considering your UINT_MAX >> example above. Does >> >> w1 = w1.from (w1, TYPE_PRECISION (type), TYPE_SIGN (inner_type)); >> >> not work for some reason? With TYPE_SIGN (inner_type), I encountered situations like this in the testsuite (mostly Fortran but also 20000422-1.c): Folding statement: _1 = _9 + 1; Applying pattern match.pd:1214, gimple-match.c:8719 gimple_simplified to _93 = (sizetype) _8; _1 = _93 + 4294967296; Folded into: _1 = _93 + 4294967296; in _8 = (unsigned int) i_73; _5 = _8 + 4294967295; _9 = (sizetype) _5; _93 = (sizetype) _8; _1 = _93 + 4294967296; TYPE_SIGN (inner_type) is PLUS here, although it should be MINUS in order to combine -1 and +1 to 0. Perhaps this can be handled differently with keeping TYPE_SIGN (inner_type)? On the other hand, using SIGNED instead of TYPE_SIGN (inner_type) worked for all cases I looked at, like if (a > 1 && a < 10) return (unsigned long)(a + UINT_MAX) + 1; For if (a > 0 && a < 10) extract_range...() would not find a non-VR_VARYING range although we should probably still be able to simplify this. if (a > 0) Here, vrp figures out [0,4294967294] and the simplification takes place. For if (a <= 10) return (unsigned long)(a + (UINT_MAX - 10)) + 1; 003t.original already reads return (long unsigned int) a + 4294967286 From this I hand-wavingly deduced, we'd only see instances where a sign-extension of the constant is fine (test suites on s390x and x86 affirm this for what it's woth) but I don't have a cogent reason hence my doubts :) I'm ok with omitting the sign-changing case (I hadn't though of it anyway) and adapted the patch. I haven't attached the updated version though, because I'm still unsure about the issue above (despite the clean test suite). Regards Robin
Ping. Any idea how to tackle this?
On Mon, Nov 28, 2016 at 2:26 PM, Robin Dapp <rdapp@linux.vnet.ibm.com> wrote: >>> + /* Sign-extend @1 to TYPE. */ >>> + w1 = w1.from (w1, TYPE_PRECISION (type), SIGNED); >>> >>> not sure why you do always sign-extend. If the inner op is unsigned >>> and we widen then that's certainly bogus considering your UINT_MAX >>> example above. Does >>> >>> w1 = w1.from (w1, TYPE_PRECISION (type), TYPE_SIGN (inner_type)); >>> >>> not work for some reason? > > With TYPE_SIGN (inner_type), I encountered situations like this in the > testsuite (mostly Fortran but also 20000422-1.c): > > Folding statement: _1 = _9 + 1; > Applying pattern match.pd:1214, gimple-match.c:8719 > gimple_simplified to _93 = (sizetype) _8; > _1 = _93 + 4294967296; > Folded into: _1 = _93 + 4294967296; > > in > _8 = (unsigned int) i_73; > _5 = _8 + 4294967295; > _9 = (sizetype) _5; > _93 = (sizetype) _8; > _1 = _93 + 4294967296; > > TYPE_SIGN (inner_type) is PLUS here, although it should be MINUS in > order to combine -1 and +1 to 0. Perhaps this can be handled differently > with keeping TYPE_SIGN (inner_type)? So we have (uint64_t)(uint32 + -1U) + 1 and using TYPE_SIGN (inner_type) produces (uint64_t)uint32 + -1U + 1. This simply means that we cannot ignore overflow of the inner operation and for some reason your change to extract_range_from_binary_expr didn't catch this. That is _8 + 4294967295 overflows but we ignored that. > On the other hand, using SIGNED instead of TYPE_SIGN (inner_type) worked > for all cases I looked at, like > if (a > 1 && a < 10) > return (unsigned long)(a + UINT_MAX) + 1; > For > if (a > 0 && a < 10) > extract_range...() would not find a non-VR_VARYING range although we > should probably still be able to simplify this. > > if (a > 0) > Here, vrp figures out [0,4294967294] and the simplification takes place. > > For > if (a <= 10) > return (unsigned long)(a + (UINT_MAX - 10)) + 1; > 003t.original already reads > return (long unsigned int) a + 4294967286 > > From this I hand-wavingly deduced, we'd only see instances where a > sign-extension of the constant is fine (test suites on s390x and x86 > affirm this for what it's woth) but I don't have a cogent reason hence > my doubts :) Well, even if sign-extending you can probably construct a testcase where that's still wrong with respect to overflow. Richard. > I'm ok with omitting the sign-changing case (I hadn't though of it > anyway) and adapted the patch. I haven't attached the updated version > though, because I'm still unsure about the issue above (despite the > clean test suite). > > Regards > Robin >
Index: gcc/tree-ssa-propagate.c =================================================================== --- gcc/tree-ssa-propagate.c (revision 242875) +++ gcc/tree-ssa-propagate.c (working copy) @@ -1065,13 +1065,15 @@ substitute_and_fold_dom_walker::before_d /* Replace real uses in the statement. */ did_replace |= replace_uses_in (stmt, get_value_fn); + if (did_replace) + gimple_set_modified (stmt, true); /* If we made a replacement, fold the statement. */ - if (did_replace) + if (fold_stmt (&i, follow_single_use_edges)) { - fold_stmt (&i, follow_single_use_edges); stmt = gsi_stmt (i); gimple_set_modified (stmt, true); + did_replace = true; } /* Some statements may be simplified using propagator