| Message ID | 56F1837D.1000508@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 22, 2016 at 6:40 PM, Richard Henderson <rth@redhat.com> wrote: > In PR68142 you added a check for overflow + __INT_MIN__. > I can't figure out why the check for __INT_MIN__, except > that it seems specific to the test case you examined. > > And indeed, this test case shows how things go wrong > with other distributed folding leading to overflow. Huh, not sure what I was thinking .. but I remember being on a hunt through various INT_MIN related overflow bugs when running into this one. > I added two tests, one signed, one unsigned. The second > verifies that we do still fold for the defined-overflow case. > > Ok? Ok. Note that always when I find bugs in extract_muldiv and try to decipher what it does I think we need to rip that out, replacing it with some simple patterns and leaving the rest to passes like reassoc. It's simply a beast that proved to be a can of worms... Thanks, Richard. > > r~
On Wed, Mar 23, 2016 at 09:59:56AM +0100, Richard Biener wrote: > Note that always when I find bugs in extract_muldiv and try > to decipher what it does I think we need to rip that out, > replacing it with some simple patterns and leaving the rest > to passes like reassoc. It's simply a beast that proved to > be a can of worms... That is true, but sadly any optimization removals from extract_muldiv proved to be huge cans of worms too (e.g. in constexpr handling or constant initializers). Jakub
On Wed, Mar 23, 2016 at 10:53 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Mar 23, 2016 at 09:59:56AM +0100, Richard Biener wrote: >> Note that always when I find bugs in extract_muldiv and try >> to decipher what it does I think we need to rip that out, >> replacing it with some simple patterns and leaving the rest >> to passes like reassoc. It's simply a beast that proved to >> be a can of worms... > > That is true, but sadly any optimization removals from extract_muldiv > proved to be huge cans of worms too (e.g. in constexpr handling > or constant initializers). Yes, I expect it is quite some work to decipher what this beast actually does and what parts of it do not have an equivalent on the GIMPLE level as well. I'm not sure if constexprs or constnat initializers need to handle arbitrary association for example. For example int a, b; constexpr int c = (((a + b) - a) - b); doesn't appear to be a valid constexpr. So for that (and the constant initializer case) I don't see the issue really unless it is coping with pointer arithmetic lowering the FEs perform and symbolic constants. And for those cases the FEs should simply avoid the lowering (and thus the ME have appropriate representation and lower itself at some point). Richard. > Jakub
On 03/22/2016 11:40 AM, Richard Henderson wrote: > In PR68142 you added a check for overflow + __INT_MIN__. > I can't figure out why the check for __INT_MIN__, except > that it seems specific to the test case you examined. > > And indeed, this test case shows how things go wrong > with other distributed folding leading to overflow. > > I added two tests, one signed, one unsigned. The second > verifies that we do still fold for the defined-overflow case. > > Ok? Richi ack'd. I went ahead and committed this to the trunk. jeff
diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 9d861c6..44fe2a2 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -6116,11 +6116,9 @@ extract_muldiv_1 (tree t, tree c, enum tree_code code, tree wide_type, { tree tem = const_binop (code, fold_convert (ctype, t), fold_convert (ctype, c)); - /* If the multiplication overflowed to INT_MIN then we lost sign - information on it and a subsequent multiplication might - spuriously overflow. See PR68142. */ - if (TREE_OVERFLOW (tem) - && wi::eq_p (tem, wi::min_value (TYPE_PRECISION (ctype), SIGNED))) + /* If the multiplication overflowed, we lost information on it. + See PR68142 and PR69845. */ + if (TREE_OVERFLOW (tem)) return NULL_TREE; return tem; } diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr69845-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr69845-1.c new file mode 100644 index 0000000..92927ba --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr69845-1.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target int32 } */ +/* { dg-options "-O -fdump-tree-gimple -fdump-tree-optimized" } */ + +int +main () +{ + struct S { char s; } v; + v.s = 47; + int a = (int) v.s; + int b = (27005061 + (a + 680455)); + int c = ((1207142401 * (((8 * b) + 9483541) - 230968044)) + 469069442); + if (c != 1676211843) + __builtin_abort (); + return 0; +} + +/* { dg-final { scan-tree-dump-times "b \\\* 8" 1 "gimple" } } */ +/* { dg-final { scan-tree-dump-not "abort" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr69845-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr69845-2.c new file mode 100644 index 0000000..e0b38e9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr69845-2.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target int32 } */ +/* { dg-options "-O -fdump-tree-gimple -fdump-tree-optimized" } */ + +int +main () +{ + struct S { char s; } v; + v.s = 47; + unsigned int a = (unsigned int) v.s; + unsigned int b = (27005061 + (a + 680455)); + unsigned int c + = ((1207142401u * (((8u * b) + 9483541u) - 230968044u)) + 469069442u); + if (c != 1676211843u) + __builtin_abort (); + return 0; +} + +/* { dg-final { scan-tree-dump-times "b \\\* 1067204616" 1 "gimple" } } */ +/* { dg-final { scan-tree-dump-not "abort" "optimized" } } */