RFC: allowing fold to change location of args (PR/41451)

RFC: allowing fold to change location of args (PR/41451)

[Aldy Hernandez]

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

Re: RFC: allowing fold to change location of args (PR/41451)

[Richard Guenther]

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.

Re: RFC: allowing fold to change location of args (PR/41451)

[Aldy Hernandez]

> > ? ? ?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

Re: RFC: allowing fold to change location of args (PR/41451)

[Andrew Pinski]

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

Re: RFC: allowing fold to change location of args (PR/41451)

[Richard Guenther]

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.

Re: RFC: allowing fold to change location of args (PR/41451)

[Aldy Hernandez]

> 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;
 	}

Re: RFC: allowing fold to change location of args (PR/41451)

[Richard Guenther]

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;
>        }
>

Re: RFC: allowing fold to change location of args (PR/41451)

[Aldy Hernandez]

> 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;
 	}

Re: RFC: allowing fold to change location of args (PR/41451)

[Richard Guenther]

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;
>        }
>

Re: RFC: allowing fold to change location of args (PR/41451)

[Jakub Jelinek]

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