Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts

[Martin Sebor <msebor@gmail.com>]

On 5/7/21 4:21 AM, Richard Biener via Gcc-patches wrote:
> On Fri, May 7, 2021 at 12:17 PM Richard Biener <rguenther@suse.de> wrote:
>>
>> canonicalize_constructor_val was setting TREE_ADDRESSABLE on bases
>> of ADDR_EXPRs but that's futile when we're dealing with CTOR values
>> in debug stmts.  This rips out the code which was added for Java
>> and should have been an assertion when we didn't have debug stmts.
>>
>> Bootstrapped and tested on x86_64-unknown-linux-gnu for all languages
>> which revealed PR100468 for which I added the cp/class.c hunk below.
>> Re-testing with that in progress.
>>
>> OK for trunk and branch?  It looks like this C++ code is new in GCC 11.
> 
> I mislooked, the code is old.
> 
> This hunk also breaks (or fixes) g++.dg/tree-ssa/array-temp1.C where
> the gimplifier previously passes the
> 
>             && (flag_merge_constants >= 2 || !TREE_ADDRESSABLE (object))
> 
> check guarding it against unifying addresses of different instances
> of variables.  Clearly in the case of the testcase there are addresses to
> this variable as part of the initializer list construction.  So the hunk fixes
> wrong-code, but it breaks the testcase.
> 
> Any comments?  I can of course change the testcase accordingly.

Just one belated comment.  I realize you already pushed this change
but...

> 
> Thanks,
> Richard.
> 
>> Thanks,
>> Richard.
>>
>> 2021-05-07  Richard Biener  <rguenther@suse.de>
>>
>>          PR middle-end/100464
>>          PR c++/100468
>> gcc/
>>          * gimple-fold.c (canonicalize_constructor_val): Do not set
>>          TREE_ADDRESSABLE.
>>
>> gcc/cp/
>>          * call.c (set_up_extended_ref_temp): Mark the temporary
>>          addressable if the TARGET_EXPR was.
>>
>> gcc/testsuite/
>>          * gcc.dg/pr100464.c: New testcase.
>> ---
>>   gcc/cp/call.c                   |  2 ++
>>   gcc/gimple-fold.c               |  4 +++-
>>   gcc/testsuite/gcc.dg/pr100464.c | 16 ++++++++++++++++
>>   3 files changed, 21 insertions(+), 1 deletion(-)
>>   create mode 100644 gcc/testsuite/gcc.dg/pr100464.c
>>
>> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
>> index 57bac05fe70..ea97be22f07 100644
>> --- a/gcc/cp/call.c
>> +++ b/gcc/cp/call.c
>> @@ -12478,6 +12478,8 @@ set_up_extended_ref_temp (tree decl, tree expr, vec<tree, va_gc> **cleanups,
>>        VAR.  */
>>     if (TREE_CODE (expr) != TARGET_EXPR)
>>       expr = get_target_expr (expr);
>> +  else if (TREE_ADDRESSABLE (expr))
>> +    TREE_ADDRESSABLE (var) = 1;
>>
>>     if (TREE_CODE (decl) == FIELD_DECL
>>         && extra_warnings && !TREE_NO_WARNING (decl))
>> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
>> index aa33779b753..768ef89d876 100644
>> --- a/gcc/gimple-fold.c
>> +++ b/gcc/gimple-fold.c
>> @@ -245,7 +245,9 @@ canonicalize_constructor_val (tree cval, tree from_decl)
>>         if (TREE_TYPE (base) == error_mark_node)
>>          return NULL_TREE;
>>         if (VAR_P (base))
>> -       TREE_ADDRESSABLE (base) = 1;
>> +       /* ???  We should be able to assert that TREE_ADDRESSABLE is set,
>> +          but since the use can be in a debug stmt we can't.  */
>> +       ;

...as I mentioned before, I find these question marks confusing
(and there are over a thousand instances of them in GCC sources).
They bring the comment that follows into question.  Please consider
spelling out in words what you mean instead.  (E.g., use FIXME: We
should be able to assert...)

Martin

>>         else if (TREE_CODE (base) == FUNCTION_DECL)
>>          {
>>            /* Make sure we create a cgraph node for functions we'll reference.
>> diff --git a/gcc/testsuite/gcc.dg/pr100464.c b/gcc/testsuite/gcc.dg/pr100464.c
>> new file mode 100644
>> index 00000000000..46cc37dff54
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr100464.c
>> @@ -0,0 +1,16 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O3 -fcompare-debug" } */
>> +
>> +int *a;
>> +static int b, c, d, e, g, h;
>> +int f;
>> +void i() {
>> +  int *j[] = {&e, &b, &b, &d, &b, &b, &g, &e, &g, &b, &b,
>> +              &b, &b, &g, &e, &e, &b, &b, &d, &b, &b, &e,
>> +              &e, &g, &b, &b, &b, &b, &g, &e, &g, &c, &e};
>> +  int **k = &j[5];
>> +  for (; f;)
>> +    b |= *a;
>> +  *k = &h;
>> +}
>> +int main() {}
>> --
>> 2.26.2