* RFC: allowing fold to change location of args (PR/41451)
@ 2009-10-26 16:13 Aldy Hernandez
2009-10-26 16:40 ` Richard Guenther
0 siblings, 1 reply; 10+ messages in thread
From: Aldy Hernandez @ 2009-10-26 16:13 UTC (permalink / raw)
To: gcc
Hi folks.
In this PR the problem is that a call to fold_build2_loc() returns one
of the original arguments unchanged. In the code below we take this
result and change its location before returning it.
tem = fold_build2_loc (loc, code, type,
fold_convert_loc (loc, TREE_TYPE (op0),
TREE_OPERAND (arg0, 1)), op1);
protected_set_expr_location (tem, loc);
When --enable-checking=fold, fold verifies that none of the arguments
changed, which in this case it obviously does.
Would be ok to allow a TREE_EXP's location to change within fold? That is,
add locus (for TREE_EXP's) to the list of allowed changeable fields in
fold_checksum_tree()?
Aldy
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 16:13 RFC: allowing fold to change location of args (PR/41451) Aldy Hernandez @ 2009-10-26 16:40 ` Richard Guenther 2009-10-26 16:41 ` Aldy Hernandez 0 siblings, 1 reply; 10+ messages in thread From: Richard Guenther @ 2009-10-26 16:40 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc On Mon, Oct 26, 2009 at 4:39 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > Hi folks. > > In this PR the problem is that a call to fold_build2_loc() returns one > of the original arguments unchanged. In the code below we take this > result and change its location before returning it. > > tem = fold_build2_loc (loc, code, type, > fold_convert_loc (loc, TREE_TYPE (op0), > TREE_OPERAND (arg0, 1)), op1); > protected_set_expr_location (tem, loc); > > When --enable-checking=fold, fold verifies that none of the arguments > changed, which in this case it obviously does. > > Would be ok to allow a TREE_EXP's location to change within fold? That is, > add locus (for TREE_EXP's) to the list of allowed changeable fields in > fold_checksum_tree()? Why? It looks like the above should already take care of the correct location via passing loc to fold_build2_loc. Richard. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 16:40 ` Richard Guenther @ 2009-10-26 16:41 ` Aldy Hernandez 2009-10-26 17:29 ` Andrew Pinski 2009-10-26 17:31 ` Richard Guenther 0 siblings, 2 replies; 10+ messages in thread From: Aldy Hernandez @ 2009-10-26 16:41 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc > > ? ? ?tem = fold_build2_loc (loc, code, type, > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? fold_convert_loc (loc, TREE_TYPE (op0), > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TREE_OPERAND (arg0, 1)), op1); > > ? ? ?protected_set_expr_location (tem, loc); > > > > When --enable-checking=fold, fold verifies that none of the arguments > > changed, which in this case it obviously does. > > > > Would be ok to allow a TREE_EXP's location to change within fold? ?That is, > > add locus (for TREE_EXP's) to the list of allowed changeable fields in > > fold_checksum_tree()? > > Why? It looks like the above should already take care of the > correct location via passing loc to fold_build2_loc. The correct location is being passed. The problem is that in the testcase in question, we modify the original statement (with the new locus). Modifying the original statement is a no no in fold_checksum_tree. We have two options: a) Allow locus changes in fold_checksum_tree. b) Fix fold-const throughout to make a copy of the result of fold_build* calls if we're about to change it's location-- in case fold is returning any of the original arguments. IMO (b) is too inefficient when we can easily do (a). Aldy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 16:41 ` Aldy Hernandez @ 2009-10-26 17:29 ` Andrew Pinski 2009-10-27 20:19 ` Jakub Jelinek 2009-10-26 17:31 ` Richard Guenther 1 sibling, 1 reply; 10+ messages in thread From: Andrew Pinski @ 2009-10-26 17:29 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Richard Guenther, gcc On Mon, Oct 26, 2009 at 9:37 AM, Aldy Hernandez <aldyh@redhat.com> wrote: > We have two options: > > a) Allow locus changes in fold_checksum_tree. > b) Fix fold-const throughout to make a copy of the result of fold_build* > calls if we're about to change it's location-- in case fold is returning > any of the original arguments. > > IMO (b) is too inefficient when we can easily do (a). Except (b) is correct as fold should not be modifiying the tree at all. That is the whole point of fold_checksum_tree. We allow some things to change like TYPE_VARIANTs and such but we should not allow a huge change like changing the locus on a statement inside fold. Thanks, Andrew Pinski ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 17:29 ` Andrew Pinski @ 2009-10-27 20:19 ` Jakub Jelinek 0 siblings, 0 replies; 10+ messages in thread From: Jakub Jelinek @ 2009-10-27 20:19 UTC (permalink / raw) To: Andrew Pinski; +Cc: Aldy Hernandez, Richard Guenther, gcc On Mon, Oct 26, 2009 at 09:40:06AM -0700, Andrew Pinski wrote: > Except (b) is correct as fold should not be modifiying the tree at > all. That is the whole point of fold_checksum_tree. We allow some > things to change like TYPE_VARIANTs and such but we should not allow a The TYPE_VARIANTs case is just that a new variant can be added to a type, but that's not a change of the type. Similarly with other chains. The only actual change we allow is that a decl might have DECL_ASSEMBLER_NAME set during fold. Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 16:41 ` Aldy Hernandez 2009-10-26 17:29 ` Andrew Pinski @ 2009-10-26 17:31 ` Richard Guenther 2009-10-26 19:57 ` Aldy Hernandez 1 sibling, 1 reply; 10+ messages in thread From: Richard Guenther @ 2009-10-26 17:31 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc On Mon, Oct 26, 2009 at 5:37 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> > ? ? ?tem = fold_build2_loc (loc, code, type, >> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? fold_convert_loc (loc, TREE_TYPE (op0), >> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TREE_OPERAND (arg0, 1)), op1); >> > ? ? ?protected_set_expr_location (tem, loc); >> > >> > When --enable-checking=fold, fold verifies that none of the arguments >> > changed, which in this case it obviously does. >> > >> > Would be ok to allow a TREE_EXP's location to change within fold? ?That is, >> > add locus (for TREE_EXP's) to the list of allowed changeable fields in >> > fold_checksum_tree()? >> >> Why? It looks like the above should already take care of the >> correct location via passing loc to fold_build2_loc. > > The correct location is being passed. The problem is that in the > testcase in question, we modify the original statement (with the new > locus). Modifying the original statement is a no no in > fold_checksum_tree. That wasn't my question. tem = fold_build2_loc (loc, code, type, fold_convert_loc (loc, TREE_TYPE (op0), TREE_OPERAND (arg0, 1)), op1); protected_set_expr_location (tem, loc); here tem is built by calling fold_build2_loc. So why is the location of tem not loc after that. This sounds like the actual bug (and the protected_set_expr_location should be redundant). Richard. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 17:31 ` Richard Guenther @ 2009-10-26 19:57 ` Aldy Hernandez 2009-10-26 21:44 ` Richard Guenther 0 siblings, 1 reply; 10+ messages in thread From: Aldy Hernandez @ 2009-10-26 19:57 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc, gcc-patches > That wasn't my question. > > tem = fold_build2_loc (loc, code, type, > fold_convert_loc (loc, TREE_TYPE (op0), > TREE_OPERAND (arg0, 1)), op1); > protected_set_expr_location (tem, loc); > > here tem is built by calling fold_build2_loc. So why is the location > of tem not loc after that. This sounds like the actual bug > (and the protected_set_expr_location should be redundant). Indeed, the culprit is fold_convert_loc which is not setting the location. OK for trunk pending tests? PR bootstrap/41451 * fold-const.c (fold_convert_loc): Return a new node if all we're going to change is the location. (fold_binary_loc): Do not call protected_set_expr_location if unecessary. Index: fold-const.c =================================================================== --- fold-const.c (revision 153549) +++ fold-const.c (working copy) @@ -2635,7 +2635,13 @@ fold_convert_loc (location_t loc, tree t tree tem; if (type == orig) - return arg; + { + if (!CAN_HAVE_LOCATION_P (arg) || EXPR_LOCATION (arg) == loc) + return arg; + arg = copy_node (arg); + SET_EXPR_LOCATION (arg, loc); + return arg; + } if (TREE_CODE (arg) == ERROR_MARK || TREE_CODE (type) == ERROR_MARK @@ -10134,7 +10140,6 @@ fold_binary_loc (location_t loc, tem = fold_build2_loc (loc, code, type, fold_convert_loc (loc, TREE_TYPE (op0), TREE_OPERAND (arg0, 1)), op1); - protected_set_expr_location (tem, loc); tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0), tem); goto fold_binary_exit; } @@ -10144,7 +10149,6 @@ fold_binary_loc (location_t loc, tem = fold_build2_loc (loc, code, type, op0, fold_convert_loc (loc, TREE_TYPE (op1), TREE_OPERAND (arg1, 1))); - protected_set_expr_location (tem, loc); tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0), tem); goto fold_binary_exit; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 19:57 ` Aldy Hernandez @ 2009-10-26 21:44 ` Richard Guenther 2009-10-26 22:10 ` Aldy Hernandez 0 siblings, 1 reply; 10+ messages in thread From: Richard Guenther @ 2009-10-26 21:44 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc, gcc-patches On Mon, Oct 26, 2009 at 6:28 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> That wasn't my question. >> >> tem = fold_build2_loc (loc, code, type, >> fold_convert_loc (loc, TREE_TYPE (op0), >> TREE_OPERAND (arg0, 1)), op1); >> protected_set_expr_location (tem, loc); >> >> here tem is built by calling fold_build2_loc. So why is the location >> of tem not loc after that. This sounds like the actual bug >> (and the protected_set_expr_location should be redundant). > > Indeed, the culprit is fold_convert_loc which is not setting the location. > > OK for trunk pending tests? Certainly better. But I fail to see why a different location would be better than the original here. I assume all tokens have a correct initial location. Then why is for example for int i; in (int) i the location of the conversion a better location than the one of i in the folded result? Thus, why not throw protected_set_expr_location in the bit-bucket completely? Richard. > PR bootstrap/41451 > * fold-const.c (fold_convert_loc): Return a new node if all we're > going to change is the location. > (fold_binary_loc): Do not call protected_set_expr_location if > unecessary. > > Index: fold-const.c > =================================================================== > --- fold-const.c (revision 153549) > +++ fold-const.c (working copy) > @@ -2635,7 +2635,13 @@ fold_convert_loc (location_t loc, tree t > tree tem; > > if (type == orig) > - return arg; > + { > + if (!CAN_HAVE_LOCATION_P (arg) || EXPR_LOCATION (arg) == loc) > + return arg; > + arg = copy_node (arg); > + SET_EXPR_LOCATION (arg, loc); > + return arg; > + } > > if (TREE_CODE (arg) == ERROR_MARK > || TREE_CODE (type) == ERROR_MARK > @@ -10134,7 +10140,6 @@ fold_binary_loc (location_t loc, > tem = fold_build2_loc (loc, code, type, > fold_convert_loc (loc, TREE_TYPE (op0), > TREE_OPERAND (arg0, 1)), op1); > - protected_set_expr_location (tem, loc); > tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0), tem); > goto fold_binary_exit; > } > @@ -10144,7 +10149,6 @@ fold_binary_loc (location_t loc, > tem = fold_build2_loc (loc, code, type, op0, > fold_convert_loc (loc, TREE_TYPE (op1), > TREE_OPERAND (arg1, 1))); > - protected_set_expr_location (tem, loc); > tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0), tem); > goto fold_binary_exit; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 21:44 ` Richard Guenther @ 2009-10-26 22:10 ` Aldy Hernandez 2009-10-26 23:07 ` Richard Guenther 0 siblings, 1 reply; 10+ messages in thread From: Aldy Hernandez @ 2009-10-26 22:10 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc, gcc-patches > Certainly better. But I fail to see why a different location would be > better than the original here. I assume all tokens have a correct initial > location. Then why is for example for int i; in (int) i the location of > the conversion a better location than the one of i in the folded result? I don't care either way. OK pending tests? PR bootstrap/41451 * fold-const.c (fold_binary_loc): Do not call protected_set_expr_location. Index: fold-const.c =================================================================== --- fold-const.c (revision 153549) +++ fold-const.c (working copy) @@ -10134,7 +10134,6 @@ fold_binary_loc (location_t loc, tem = fold_build2_loc (loc, code, type, fold_convert_loc (loc, TREE_TYPE (op0), TREE_OPERAND (arg0, 1)), op1); - protected_set_expr_location (tem, loc); tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0), tem); goto fold_binary_exit; } @@ -10144,7 +10143,6 @@ fold_binary_loc (location_t loc, tem = fold_build2_loc (loc, code, type, op0, fold_convert_loc (loc, TREE_TYPE (op1), TREE_OPERAND (arg1, 1))); - protected_set_expr_location (tem, loc); tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0), tem); goto fold_binary_exit; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: allowing fold to change location of args (PR/41451) 2009-10-26 22:10 ` Aldy Hernandez @ 2009-10-26 23:07 ` Richard Guenther 0 siblings, 0 replies; 10+ messages in thread From: Richard Guenther @ 2009-10-26 23:07 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc, gcc-patches On Mon, Oct 26, 2009 at 10:42 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> Certainly better. But I fail to see why a different location would be >> better than the original here. I assume all tokens have a correct initial >> location. Then why is for example for int i; in (int) i the location of >> the conversion a better location than the one of i in the folded result? > > I don't care either way. > > OK pending tests? Ok. Thanks, Richard. > PR bootstrap/41451 > * fold-const.c (fold_binary_loc): Do not call > protected_set_expr_location. > > Index: fold-const.c > =================================================================== > --- fold-const.c (revision 153549) > +++ fold-const.c (working copy) > @@ -10134,7 +10134,6 @@ fold_binary_loc (location_t loc, > tem = fold_build2_loc (loc, code, type, > fold_convert_loc (loc, TREE_TYPE (op0), > TREE_OPERAND (arg0, 1)), op1); > - protected_set_expr_location (tem, loc); > tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0), tem); > goto fold_binary_exit; > } > @@ -10144,7 +10143,6 @@ fold_binary_loc (location_t loc, > tem = fold_build2_loc (loc, code, type, op0, > fold_convert_loc (loc, TREE_TYPE (op1), > TREE_OPERAND (arg1, 1))); > - protected_set_expr_location (tem, loc); > tem = build2 (COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0), tem); > goto fold_binary_exit; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-10-27 20:18 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-10-26 16:13 RFC: allowing fold to change location of args (PR/41451) Aldy Hernandez 2009-10-26 16:40 ` Richard Guenther 2009-10-26 16:41 ` Aldy Hernandez 2009-10-26 17:29 ` Andrew Pinski 2009-10-27 20:19 ` Jakub Jelinek 2009-10-26 17:31 ` Richard Guenther 2009-10-26 19:57 ` Aldy Hernandez 2009-10-26 21:44 ` Richard Guenther 2009-10-26 22:10 ` Aldy Hernandez 2009-10-26 23:07 ` Richard Guenther
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).