| Message ID | AANLkTi=UHGM2JDihLE6+cdz9EMod6-4VWPjBjCBn6YXJ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Nov 21, 2010 at 8:02 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Sun, Nov 21, 2010 at 7:43 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Sat, Nov 20, 2010 at 8:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >>> Hello! >>> >>> Attached patch fixes PR 43057 by unsharing arg0 argument. >>> >>> 2010-11-20 Uros Bizjak <ubizjak@gmail.com> >>> >>> PR middle-end/43057 >>> * fold-const.c (fold_ternary_loc) <COND_EXPR>: Unshare arg0 when >>> converting A ? 1 : 0 to A. >>> >>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu >>> {,-m32}. FWIW, "--enable-checking=all" bootstrap is still running and >>> it already compiled up to stage-2 gcc/dse.o. I will leave the >>> bootstrap overnight to see if it breaks (it probably won't). >>> >>> OK for mainline and 4.5? >> >> Ok. > > Actually, --enable-checking=all build broke in fold-const.c, line > 13361 (trying to build libstdc++/src/debug.cc) in the same call to > pedantic_non_lvalue_loc. This points to the problem in the called > function itself. > > Attached patch unshares the expression in the pedantic_non_lvalue_loc > itself. We shouldn't change the original operand by setting the > location. > > 2010-11-21 Uros Bizjak <ubizjak@gmail.com> > > PR middle-end/43057 > * fold-const.c (pedantic_non_lvalue_loc): Unshare x before > setting location. > > OK for mainline and 4.5? Ok. Thanks, Richard. > Uros. > > Index: fold-const.c > =================================================================== > --- fold-const.c (revision 166974) > +++ fold-const.c (working copy) > @@ -2097,6 +2097,7 @@ > { > if (pedantic_lvalues) > return non_lvalue_loc (loc, x); > + x = unshare_expr (x); > protected_set_expr_location (x, loc); > return x; > } >
On Sun, Nov 21, 2010 at 09:48:54PM +0100, Richard Guenther wrote: > > Actually, --enable-checking=all build broke in fold-const.c, line > > 13361 (trying to build libstdc++/src/debug.cc) in the same call to > > pedantic_non_lvalue_loc. This points to the problem in the called > > function itself. > > > > Attached patch unshares the expression in the pedantic_non_lvalue_loc > > itself. We shouldn't change the original operand by setting the > > location. > > > > 2010-11-21 Uros Bizjak <ubizjak@gmail.com> > > > > PR middle-end/43057 > > * fold-const.c (pedantic_non_lvalue_loc): Unshare x before > > setting location. > > > > OK for mainline and 4.5? > > Ok. I don't think this is desirable. 1) unshare_expr won't unshare SAVE_EXPR/TARGET_EXPR/BIND_EXPR 2) you unshare even when there is no reason for it (!CAN_HAVE_LOCATION_P (x) or when it already has the desired locus) 3) you don't need to do a deep unshare just to set location So, I think it would be much better to do something like: if (CAN_HAVE_LOCATION_P (x) && EXPR_LOCATION (x) != loc && TREE_CODE (x) != SAVE_EXPR && TREE_CODE (x) != TARGET_EXPR && TREE_CODE (x) != BIND_EXPR) { x = copy_node (x); SET_EXPR_LOCATION (x, loc); } Also, as I said in the PR, this isn't the only place in fold-const.c that needs fixing for --enable-checking=fold, there are more than 10 similar spots. And buildN_loc should be introduced and used where possible. Jakub
On Sun, Nov 21, 2010 at 10:13 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Sun, Nov 21, 2010 at 09:48:54PM +0100, Richard Guenther wrote: >> > Actually, --enable-checking=all build broke in fold-const.c, line >> > 13361 (trying to build libstdc++/src/debug.cc) in the same call to >> > pedantic_non_lvalue_loc. This points to the problem in the called >> > function itself. >> > >> > Attached patch unshares the expression in the pedantic_non_lvalue_loc >> > itself. We shouldn't change the original operand by setting the >> > location. >> > >> > 2010-11-21 Uros Bizjak <ubizjak@gmail.com> >> > >> > PR middle-end/43057 >> > * fold-const.c (pedantic_non_lvalue_loc): Unshare x before >> > setting location. >> > >> > OK for mainline and 4.5? >> >> Ok. > > I don't think this is desirable. > 1) unshare_expr won't unshare SAVE_EXPR/TARGET_EXPR/BIND_EXPR > 2) you unshare even when there is no reason for it (!CAN_HAVE_LOCATION_P > (x) or when it already has the desired locus) > 3) you don't need to do a deep unshare just to set location > > So, I think it would be much better to do something like: > if (CAN_HAVE_LOCATION_P (x) > && EXPR_LOCATION (x) != loc > && TREE_CODE (x) != SAVE_EXPR > && TREE_CODE (x) != TARGET_EXPR > && TREE_CODE (x) != BIND_EXPR) > { > x = copy_node (x); > SET_EXPR_LOCATION (x, loc); > } > > Also, as I said in the PR, this isn't the only place in fold-const.c that > needs fixing for --enable-checking=fold, there are more than 10 similar > spots. And buildN_loc should be introduced and used where possible. Your proposed approach also works (and fixes bootstrap with --enable-checking=all, too). If there are no objections from Richi, I propose to commit this change to mainline (since it enables bootstrap with --enable-checking=fold) and fix all fold fails from the testsuite separately. Thanks, Uros.
On Mon, Nov 22, 2010 at 09:34:11AM +0100, Uros Bizjak wrote: > Your proposed approach also works (and fixes bootstrap with > --enable-checking=all, too). If there are no objections from Richi, I > propose to commit this change to mainline (since it enables bootstrap > with --enable-checking=fold) and fix all fold fails from the testsuite > separately. Fine with me; if Richi agrees to that, it would be better to put this into a separate inline (fold-const.c local is ok) though, as it might be used later on in multiple locations. And then at the next phase get rid of all the protected_set_expr_location and SET_EXPR_LOCATION uses in fold-const.c, either (when used on the result of buildN), start using buildN_loc, or call this new inline instead. Jakub
On Mon, Nov 22, 2010 at 9:34 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Sun, Nov 21, 2010 at 10:13 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Sun, Nov 21, 2010 at 09:48:54PM +0100, Richard Guenther wrote: >>> > Actually, --enable-checking=all build broke in fold-const.c, line >>> > 13361 (trying to build libstdc++/src/debug.cc) in the same call to >>> > pedantic_non_lvalue_loc. This points to the problem in the called >>> > function itself. >>> > >>> > Attached patch unshares the expression in the pedantic_non_lvalue_loc >>> > itself. We shouldn't change the original operand by setting the >>> > location. >>> > >>> > 2010-11-21 Uros Bizjak <ubizjak@gmail.com> >>> > >>> > PR middle-end/43057 >>> > * fold-const.c (pedantic_non_lvalue_loc): Unshare x before >>> > setting location. >>> > >>> > OK for mainline and 4.5? >>> >>> Ok. >> >> I don't think this is desirable. >> 1) unshare_expr won't unshare SAVE_EXPR/TARGET_EXPR/BIND_EXPR >> 2) you unshare even when there is no reason for it (!CAN_HAVE_LOCATION_P >> (x) or when it already has the desired locus) >> 3) you don't need to do a deep unshare just to set location >> >> So, I think it would be much better to do something like: >> if (CAN_HAVE_LOCATION_P (x) >> && EXPR_LOCATION (x) != loc >> && TREE_CODE (x) != SAVE_EXPR >> && TREE_CODE (x) != TARGET_EXPR >> && TREE_CODE (x) != BIND_EXPR) >> { >> x = copy_node (x); >> SET_EXPR_LOCATION (x, loc); >> } >> >> Also, as I said in the PR, this isn't the only place in fold-const.c that >> needs fixing for --enable-checking=fold, there are more than 10 similar >> spots. And buildN_loc should be introduced and used where possible. > > Your proposed approach also works (and fixes bootstrap with > --enable-checking=all, too). If there are no objections from Richi, I > propose to commit this change to mainline (since it enables bootstrap > with --enable-checking=fold) and fix all fold fails from the testsuite > separately. It works for me. Richard. > Thanks, > Uros. >
Index: fold-const.c =================================================================== --- fold-const.c (revision 166974) +++ fold-const.c (working copy) @@ -2097,6 +2097,7 @@ { if (pedantic_lvalues) return non_lvalue_loc (loc, x); + x = unshare_expr (x); protected_set_expr_location (x, loc); return x; }