Message ID | 20151125143509.GU21807@redhat.com |
---|---|
State | New |
Headers | show |
On Wed, 25 Nov 2015, Marek Polacek wrote: > This is a bit tangled and I don't know how to best fix this so let me explain > in more detail. > > The gist is that a C_MAYBE_CONST_EXPR leaks into the gimplifier. > > In the testcase, we have > > (short) ((i ? *e : 0) & ~u | i & u); > > where 'e' is a pointer to volatile. During c_parser_conditional_expression, we > wrap 'i' and '*e' in C_MAYBE_CONST_EXPR. Later on, we get to build_c_cast, and > here we're trying to cast > > (c_maybe_const_expr <i> != 0 ? (unsigned int) c_maybe_const_expr <*e > : 0) > & ~u | (unsigned int) i & u > > to "short", so we call convert()->convert_to_integer() etc. As a part of this > conversion, we try to fold the expr. Now, we match this pattern: > > (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x > > Look how 'x' is duplicated in the result of the pattern, and since (because of > the volatile 'e') it has TREE_SIDE_EFFECTS, we need to wrap it in a SAVE_EXPR, > which means the convert() produces > > (short int) (((SAVE_EXPR <c_maybe_const_expr <i> != 0 ? (unsigned short) > c_maybe_const_expr <*e > : 0>) > ^ (unsigned short) i) & (unsigned short) u > ^ (SAVE_EXPR <c_maybe_const_expr <i > != 0 ? (unsigned short) > c_maybe_const_expr <*e > : 0>)) > > What's important is that we end up with a C_MAYBE_CONST_EXPR in a SAVE_EXPR. > > Down the line, we get into c_process_expr_stmt, where there's > > expr = c_fully_fold (expr, false, NULL); > > so we try to fully-fold the whole expression. However, c_fully_fold just > gives up when it sees a SAVE_EXPR, so it doesn't scrub out C_MAYBE_CONST_EXPR. > It then leaks into the gimplifier and crashes. > > This pattern is probably the only one that is problematical in this regard. > Looking more at this pattern, it looks dubious, because imagine the 'x' in > the pattern is something complex, e.g. a huge COND_EXPR or somesuch -- in > that case duplicating the 'x' does more harm than good. But the whole point of the SAVE_EXPR is that it does _not_ "duplicate" it, it just creates another use of the same value. > So after all, I > think this transformation should be restricted a bit, and only kick in when > 'x' is a constant, an SSA_NAME, or a certain DECL_P. Seems simple_operand_p > in fold-const.c was what I was after, so I've just used that. > > With this patch, we won't create those SAVE_EXPRs so c_fully_fold is able to > get rid of the C_MAYBE_CONST_EXPRs and the gimplifier is happy. > > Thoughts? No. If c_fully_fold can't handle SAVE_EXPRs then maybe c_gimplify_expr can simply strip them. Richard. > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-11-25 Marek Polacek <polacek@redhat.com> > > PR c/68513 > * fold-const.c (simple_operand_p): Export. > * fold-const.h (simple_operand_p): Declare. > * match.pd ((x & ~m) | (y & m)): Guard with simple_operand_p. > > * gcc.dg/torture/pr68513.c: New test. > > diff --git gcc/fold-const.c gcc/fold-const.c > index 698062e..9bb3a53 100644 > --- gcc/fold-const.c > +++ gcc/fold-const.c > @@ -122,7 +122,6 @@ static tree decode_field_reference (location_t, tree, HOST_WIDE_INT *, > HOST_WIDE_INT *, > machine_mode *, int *, int *, int *, > tree *, tree *); > -static int simple_operand_p (const_tree); > static bool simple_operand_p_2 (tree); > static tree range_binop (enum tree_code, tree, tree, int, tree, int); > static tree range_predecessor (tree); > @@ -4059,10 +4058,10 @@ sign_bit_p (tree exp, const_tree val) > return NULL_TREE; > } > > -/* Subroutine for fold_truth_andor_1: determine if an operand is simple enough > - to be evaluated unconditionally. */ > +/* Determine if an operand is simple enough to be evaluated > + unconditionally. */ > > -static int > +bool > simple_operand_p (const_tree exp) > { > /* Strip any conversions that don't change the machine mode. */ > diff --git gcc/fold-const.h gcc/fold-const.h > index 7741802..717c840 100644 > --- gcc/fold-const.h > +++ gcc/fold-const.h > @@ -181,6 +181,7 @@ extern tree const_unop (enum tree_code, tree, tree); > extern tree const_binop (enum tree_code, tree, tree, tree); > extern bool negate_mathfn_p (combined_fn); > extern const char *c_getstr (tree); > +extern bool simple_operand_p (const_tree); > > /* Return OFF converted to a pointer offset type suitable as offset for > POINTER_PLUS_EXPR. Use location LOC for this conversion. */ > diff --git gcc/match.pd gcc/match.pd > index e86cc8b..4aee4a6 100644 > --- gcc/match.pd > +++ gcc/match.pd > @@ -877,7 +877,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > /* (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x */ > (simplify > (bit_ior:c (bit_and:cs @0 (bit_not @2)) (bit_and:cs @1 @2)) > - (bit_xor (bit_and (bit_xor @0 @1) @2) @0)) > + (if (simple_operand_p (@0)) > + (bit_xor (bit_and (bit_xor @0 @1) @2) @0))) > > /* Fold A - (A & B) into ~B & A. */ > (simplify > diff --git gcc/testsuite/gcc.dg/torture/pr68513.c gcc/testsuite/gcc.dg/torture/pr68513.c > index e69de29..4e08b29 100644 > --- gcc/testsuite/gcc.dg/torture/pr68513.c > +++ gcc/testsuite/gcc.dg/torture/pr68513.c > @@ -0,0 +1,13 @@ > +/* PR c/68513 */ > +/* { dg-do compile } */ > + > +int i; > +unsigned u; > +volatile unsigned int *e; > + > +void > +fn1 (void) > +{ > + (short) ((i ? *e : 0) & ~u | i & u); > + (short) (((0, 0) ? *e : 0) & ~u | i & u); > +} > > Marek > >
On Wed, Nov 25, 2015 at 03:35:10PM +0100, Marek Polacek wrote: > Look how 'x' is duplicated in the result of the pattern, and since (because of > the volatile 'e') it has TREE_SIDE_EFFECTS, we need to wrap it in a SAVE_EXPR, > which means the convert() produces > > (short int) (((SAVE_EXPR <c_maybe_const_expr <i> != 0 ? (unsigned short) > c_maybe_const_expr <*e > : 0>) > ^ (unsigned short) i) & (unsigned short) u > ^ (SAVE_EXPR <c_maybe_const_expr <i > != 0 ? (unsigned short) > c_maybe_const_expr <*e > : 0>)) > > What's important is that we end up with a C_MAYBE_CONST_EXPR in a SAVE_EXPR. > > Down the line, we get into c_process_expr_stmt, where there's > > expr = c_fully_fold (expr, false, NULL); > > so we try to fully-fold the whole expression. However, c_fully_fold just > gives up when it sees a SAVE_EXPR, so it doesn't scrub out C_MAYBE_CONST_EXPR. > It then leaks into the gimplifier and crashes. Ran into similar issues many times in the past, the main issue is that the C FE wants that before c_fully_fold is called on some expression that nobody wraps anything into a SAVE_EXPR except by calling c_save_expr. So, unless we get rid of that (severe) limitation, we should make sure that either match.pd never creates SAVE_EXPRs when called from the "early" spots in the C FE, or that it uses c_save_expr instead of save_expr. But finding out if you have an expression on which c_fully_fold has been already called or not is non-trivial too. Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say whether c_fully_fold_internal has already processed it or not, and just get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but only if that bit is not yet already set, and set it afterwards. Jakub
On Wed, 25 Nov 2015, Jakub Jelinek wrote: > On Wed, Nov 25, 2015 at 03:35:10PM +0100, Marek Polacek wrote: > > Look how 'x' is duplicated in the result of the pattern, and since (because of > > the volatile 'e') it has TREE_SIDE_EFFECTS, we need to wrap it in a SAVE_EXPR, > > which means the convert() produces > > > > (short int) (((SAVE_EXPR <c_maybe_const_expr <i> != 0 ? (unsigned short) > > c_maybe_const_expr <*e > : 0>) > > ^ (unsigned short) i) & (unsigned short) u > > ^ (SAVE_EXPR <c_maybe_const_expr <i > != 0 ? (unsigned short) > > c_maybe_const_expr <*e > : 0>)) > > > > What's important is that we end up with a C_MAYBE_CONST_EXPR in a SAVE_EXPR. > > > > Down the line, we get into c_process_expr_stmt, where there's > > > > expr = c_fully_fold (expr, false, NULL); > > > > so we try to fully-fold the whole expression. However, c_fully_fold just > > gives up when it sees a SAVE_EXPR, so it doesn't scrub out C_MAYBE_CONST_EXPR. > > It then leaks into the gimplifier and crashes. > > Ran into similar issues many times in the past, the main issue is that the C > FE wants that before c_fully_fold is called on some expression that nobody > wraps anything into a SAVE_EXPR except by calling c_save_expr. > So, unless we get rid of that (severe) limitation, we should make sure > that either match.pd never creates SAVE_EXPRs when called from the "early" > spots in the C FE, or that it uses c_save_expr instead of save_expr. > But finding out if you have an expression on which c_fully_fold has been > already called or not is non-trivial too. > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say > whether c_fully_fold_internal has already processed it or not, and just > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but > only if that bit is not yet already set, and set it afterwards. c_gimplify_expr and SAVE_EXPR_RESOLVED_P would work as well I guess? Richard.
On Wed, 25 Nov 2015, Richard Biener wrote: > On Wed, 25 Nov 2015, Jakub Jelinek wrote: > > > On Wed, Nov 25, 2015 at 03:35:10PM +0100, Marek Polacek wrote: > > > Look how 'x' is duplicated in the result of the pattern, and since (because of > > > the volatile 'e') it has TREE_SIDE_EFFECTS, we need to wrap it in a SAVE_EXPR, > > > which means the convert() produces > > > > > > (short int) (((SAVE_EXPR <c_maybe_const_expr <i> != 0 ? (unsigned short) > > > c_maybe_const_expr <*e > : 0>) > > > ^ (unsigned short) i) & (unsigned short) u > > > ^ (SAVE_EXPR <c_maybe_const_expr <i > != 0 ? (unsigned short) > > > c_maybe_const_expr <*e > : 0>)) > > > > > > What's important is that we end up with a C_MAYBE_CONST_EXPR in a SAVE_EXPR. > > > > > > Down the line, we get into c_process_expr_stmt, where there's > > > > > > expr = c_fully_fold (expr, false, NULL); > > > > > > so we try to fully-fold the whole expression. However, c_fully_fold just > > > gives up when it sees a SAVE_EXPR, so it doesn't scrub out C_MAYBE_CONST_EXPR. > > > It then leaks into the gimplifier and crashes. > > > > Ran into similar issues many times in the past, the main issue is that the C > > FE wants that before c_fully_fold is called on some expression that nobody > > wraps anything into a SAVE_EXPR except by calling c_save_expr. > > So, unless we get rid of that (severe) limitation, we should make sure > > that either match.pd never creates SAVE_EXPRs when called from the "early" > > spots in the C FE, or that it uses c_save_expr instead of save_expr. > > But finding out if you have an expression on which c_fully_fold has been > > already called or not is non-trivial too. > > > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say > > whether c_fully_fold_internal has already processed it or not, and just > > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but > > only if that bit is not yet already set, and set it afterwards. > > c_gimplify_expr and SAVE_EXPR_RESOLVED_P would work as well I guess? Or simply not call fold () from convert () -> convert_to_integer (). Richard.
On Wed, Nov 25, 2015 at 03:45:08PM +0100, Richard Biener wrote: > But the whole point of the SAVE_EXPR is that it does _not_ "duplicate" it, > it just creates another use of the same value. Of course, but when 'x' in that pattern doesn't have side-effects, it's not wrapped in SAVE_EXPR and gets duplicated, generating unnecessary code, this is when I think the pattern is harmful. > No. If c_fully_fold can't handle SAVE_EXPRs then maybe c_gimplify_expr > can simply strip them. Uhm, can we just strip SAVE_EXPRs like that? That sounds wrong. Did you mean C_MAYBE_CONST_EXPR? Marek
On Wed, Nov 25, 2015 at 03:56:39PM +0100, Richard Biener wrote: > > c_gimplify_expr and SAVE_EXPR_RESOLVED_P would work as well I guess? > > Or simply not call fold () from convert () -> convert_to_integer (). I was thinking about calling convert_to_integer_nofold in convert; that fixed the testcase I think, but I haven't done a proper testing. Marek
On Wed, 25 Nov 2015, Jakub Jelinek wrote: > But finding out if you have an expression on which c_fully_fold has been > already called or not is non-trivial too. The rule is that if the C front end needs to fold early for any reason (e.g. for warnings) then the result of folding gets wrapped in a C_MAYBE_CONST_EXPR as a signal that what's inside there doesn't need folding again - or in another kind of tree that also isn't folded inside, such as SAVE_EXPR or CALL_EXPR. > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say > whether c_fully_fold_internal has already processed it or not, and just > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but > only if that bit is not yet already set, and set it afterwards. I suppose you could do that, in line with the general principle of reducing early folding (as long as you ensure that folding the contents of a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it stay pointing to the same tree node).
On Wed, Nov 25, 2015 at 03:16:53PM +0000, Joseph Myers wrote: > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say > > whether c_fully_fold_internal has already processed it or not, and just > > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but > > only if that bit is not yet already set, and set it afterwards. > > I suppose you could do that, in line with the general principle of > reducing early folding (as long as you ensure that folding the contents of > a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it > stay pointing to the same tree node). I had a go at this, but I'm now skeptical about removing c_save_expr. save_expr calls fold (), so we need to ensure that we don't pass any C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before save_expr anyway... So maybe go the "remove C_MAYBE_CONST_EXPRs in SAVE_EXPRs in c_gimplify_expr" way? Marek
On Thu, Nov 26, 2015 at 12:15:48PM +0100, Marek Polacek wrote: > On Wed, Nov 25, 2015 at 03:16:53PM +0000, Joseph Myers wrote: > > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say > > > whether c_fully_fold_internal has already processed it or not, and just > > > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but > > > only if that bit is not yet already set, and set it afterwards. > > > > I suppose you could do that, in line with the general principle of > > reducing early folding (as long as you ensure that folding the contents of > > a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it > > stay pointing to the same tree node). > > I had a go at this, but I'm now skeptical about removing c_save_expr. > save_expr calls fold (), so we need to ensure that we don't pass any > C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before > save_expr anyway... I bet that is too hard for stage3, but IMHO if we want delayed folding, we want to delay it even for SAVE_EXPRs. Supposedly the reason why save_expr calls fold is to determine the cases where there is no point to create the SAVE_EXPR. But perhaps it should just fold for the purpose of testing that and return the original expression if after folding it is simple arithmetics, and wrap the original expression into SAVE_EXPR. Though in this particular case, where save_expr is just an optimization it is perhaps premature optimization and we should not perform that. For stage3, I agree, some other fix is needed (and one usable also for the 5 branch). Jakub
On Thu, 26 Nov 2015, Jakub Jelinek wrote: > On Thu, Nov 26, 2015 at 12:15:48PM +0100, Marek Polacek wrote: > > On Wed, Nov 25, 2015 at 03:16:53PM +0000, Joseph Myers wrote: > > > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say > > > > whether c_fully_fold_internal has already processed it or not, and just > > > > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but > > > > only if that bit is not yet already set, and set it afterwards. > > > > > > I suppose you could do that, in line with the general principle of > > > reducing early folding (as long as you ensure that folding the contents of > > > a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it > > > stay pointing to the same tree node). > > > > I had a go at this, but I'm now skeptical about removing c_save_expr. > > save_expr calls fold (), so we need to ensure that we don't pass any > > C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before > > save_expr anyway... > > I bet that is too hard for stage3, but IMHO if we want delayed folding, we > want to delay it even for SAVE_EXPRs. Supposedly the reason why save_expr > calls fold is to determine the cases where there is no point to create the > SAVE_EXPR. But perhaps it should just fold for the purpose of testing that > and return the original expression if after folding it is simple > arithmetics, and wrap the original expression into SAVE_EXPR. Btw, I tried to remove that fold () at some point but it spectacularly regressed (though before the C++ early folding work) in constexpr cases. > Though in this particular case, where save_expr is just an optimization it > is perhaps premature optimization and we should not perform that. > > For stage3, I agree, some other fix is needed (and one usable also for the 5 > branch). I'm currently testing the genmatch.c patch ... (which might make this situation even worse). I don't think we have the issue on the 5 branch so much (because of way less patterns in match.pd). Richard.
On Thu, 26 Nov 2015, Marek Polacek wrote: > I had a go at this, but I'm now skeptical about removing c_save_expr. > save_expr calls fold (), so we need to ensure that we don't pass any > C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before > save_expr anyway... > > So maybe go the "remove C_MAYBE_CONST_EXPRs in SAVE_EXPRs in c_gimplify_expr" > way? I believe it should be safe for gimplification to process C_MAYBE_CONST_EXPR in the same way c_fully_fold_internal does. That is, this should not affect correctness. If a C_MAYBE_CONST_EXPR got through to gimplification, in some cases it may mean that something did not get properly folded with c_fully_fold as it should have done - but if the move to match.pd means all optimizations currently done with fold end up working on GIMPLE as well, any missed optimizations from this should disappear (and if we can solve the diagnostics issues, eventually fewer calls to c_fully_fold should be needed and they should be more about checking what can occur in constant expressions and less about folding for optimization). The general principle of delaying folding also means that we should move away from convert_* folding things.
diff --git gcc/fold-const.c gcc/fold-const.c index 698062e..9bb3a53 100644 --- gcc/fold-const.c +++ gcc/fold-const.c @@ -122,7 +122,6 @@ static tree decode_field_reference (location_t, tree, HOST_WIDE_INT *, HOST_WIDE_INT *, machine_mode *, int *, int *, int *, tree *, tree *); -static int simple_operand_p (const_tree); static bool simple_operand_p_2 (tree); static tree range_binop (enum tree_code, tree, tree, int, tree, int); static tree range_predecessor (tree); @@ -4059,10 +4058,10 @@ sign_bit_p (tree exp, const_tree val) return NULL_TREE; } -/* Subroutine for fold_truth_andor_1: determine if an operand is simple enough - to be evaluated unconditionally. */ +/* Determine if an operand is simple enough to be evaluated + unconditionally. */ -static int +bool simple_operand_p (const_tree exp) { /* Strip any conversions that don't change the machine mode. */ diff --git gcc/fold-const.h gcc/fold-const.h index 7741802..717c840 100644 --- gcc/fold-const.h +++ gcc/fold-const.h @@ -181,6 +181,7 @@ extern tree const_unop (enum tree_code, tree, tree); extern tree const_binop (enum tree_code, tree, tree, tree); extern bool negate_mathfn_p (combined_fn); extern const char *c_getstr (tree); +extern bool simple_operand_p (const_tree); /* Return OFF converted to a pointer offset type suitable as offset for POINTER_PLUS_EXPR. Use location LOC for this conversion. */ diff --git gcc/match.pd gcc/match.pd index e86cc8b..4aee4a6 100644 --- gcc/match.pd +++ gcc/match.pd @@ -877,7 +877,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) /* (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x */ (simplify (bit_ior:c (bit_and:cs @0 (bit_not @2)) (bit_and:cs @1 @2)) - (bit_xor (bit_and (bit_xor @0 @1) @2) @0)) + (if (simple_operand_p (@0)) + (bit_xor (bit_and (bit_xor @0 @1) @2) @0))) /* Fold A - (A & B) into ~B & A. */ (simplify diff --git gcc/testsuite/gcc.dg/torture/pr68513.c gcc/testsuite/gcc.dg/torture/pr68513.c index e69de29..4e08b29 100644 --- gcc/testsuite/gcc.dg/torture/pr68513.c +++ gcc/testsuite/gcc.dg/torture/pr68513.c @@ -0,0 +1,13 @@ +/* PR c/68513 */ +/* { dg-do compile } */ + +int i; +unsigned u; +volatile unsigned int *e; + +void +fn1 (void) +{ + (short) ((i ? *e : 0) & ~u | i & u); + (short) (((0, 0) ? *e : 0) & ~u | i & u); +}