diff mbox

Fix PR79908

Message ID C270ABA6-3138-4E58-B42E-EE9E75D5F97B@linux.vnet.ibm.com
State New
Headers show

Commit Message

Bill Schmidt March 14, 2017, 2:20 p.m. UTC 
> On Mar 14, 2017, at 7:50 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> 
> 
>> On Mar 14, 2017, at 3:57 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> 
>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
>> <wschmidt@linux.vnet.ibm.com> wrote:
>>> 
>>> Index: gcc/tree-stdarg.c
>>> ===================================================================
>>> --- gcc/tree-stdarg.c   (revision 246109)
>>> +++ gcc/tree-stdarg.c   (working copy)
>>> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun)
>>>              types.  */
>>>           gimplify_assign (lhs, expr, &pre);
>>>         }
>>> -       else
>>> +       else if (is_gimple_addressable (expr))
>>>         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>> 
>> This is wrong - we lose side-effects this way and the only reason we gimplify
>> is to _not_ lose them.
>> 
>> Better is sth like
>> 
>> Index: gcc/tree-stdarg.c
>> ===================================================================
>> --- gcc/tree-stdarg.c   (revision 246082)
>> +++ gcc/tree-stdarg.c   (working copy)
>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun)
>>           gimplify_assign (lhs, expr, &pre);
>>         }
>>       else
>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>> +         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>> 
>>       input_location = saved_location;
>>       pop_gimplify_context (NULL);
> 
> OK, thanks for the explanation.  Unfortunately this fails bootstrap with a failed 
> assert a little later.  I'll dig further.

Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue
passes.  Trying this now:



> 
> Bill
>

Comments

Richard Biener March 14, 2017, 2:25 p.m. UTC | #1 
On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
>
>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>>
>>
>>> On Mar 14, 2017, at 3:57 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>>
>>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
>>> <wschmidt@linux.vnet.ibm.com> wrote:
>>>>
>>>> Index: gcc/tree-stdarg.c
>>>> ===================================================================
>>>> --- gcc/tree-stdarg.c   (revision 246109)
>>>> +++ gcc/tree-stdarg.c   (working copy)
>>>> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun)
>>>>              types.  */
>>>>           gimplify_assign (lhs, expr, &pre);
>>>>         }
>>>> -       else
>>>> +       else if (is_gimple_addressable (expr))
>>>>         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>>
>>> This is wrong - we lose side-effects this way and the only reason we gimplify
>>> is to _not_ lose them.
>>>
>>> Better is sth like
>>>
>>> Index: gcc/tree-stdarg.c
>>> ===================================================================
>>> --- gcc/tree-stdarg.c   (revision 246082)
>>> +++ gcc/tree-stdarg.c   (working copy)
>>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun)
>>>           gimplify_assign (lhs, expr, &pre);
>>>         }
>>>       else
>>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>> +         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>
>>>       input_location = saved_location;
>>>       pop_gimplify_context (NULL);
>>
>> OK, thanks for the explanation.  Unfortunately this fails bootstrap with a failed
>> assert a little later.  I'll dig further.
>
> Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue
> passes.  Trying this now:

Hmm, it should simply gimplify to a load if it's not aggregate.  Can
you share a testcase
where it doesn't work?

> Index: gcc/tree-stdarg.c
> ===================================================================
> --- gcc/tree-stdarg.c   (revision 246109)
> +++ gcc/tree-stdarg.c   (working copy)
> @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun)
>                types.  */
>             gimplify_assign (lhs, expr, &pre);
>           }
> +       else if (is_gimple_addressable (expr))

I believe any is_gimple_addressable check is flawed.

> +         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>         else
> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
> +         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue);
>
>         input_location = saved_location;
>         pop_gimplify_context (NULL);
>
>
>>
>> Bill
>>
>
Bill Schmidt March 14, 2017, 2:32 p.m. UTC | #2 
> On Mar 14, 2017, at 9:25 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
>> 
>>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>>> 
>>> 
>>>> On Mar 14, 2017, at 3:57 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>>> 
>>>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
>>>> <wschmidt@linux.vnet.ibm.com> wrote:
>>>>> 
>>>>> Index: gcc/tree-stdarg.c
>>>>> ===================================================================
>>>>> --- gcc/tree-stdarg.c   (revision 246109)
>>>>> +++ gcc/tree-stdarg.c   (working copy)
>>>>> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun)
>>>>>             types.  */
>>>>>          gimplify_assign (lhs, expr, &pre);
>>>>>        }
>>>>> -       else
>>>>> +       else if (is_gimple_addressable (expr))
>>>>>        gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>>> 
>>>> This is wrong - we lose side-effects this way and the only reason we gimplify
>>>> is to _not_ lose them.
>>>> 
>>>> Better is sth like
>>>> 
>>>> Index: gcc/tree-stdarg.c
>>>> ===================================================================
>>>> --- gcc/tree-stdarg.c   (revision 246082)
>>>> +++ gcc/tree-stdarg.c   (working copy)
>>>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun)
>>>>          gimplify_assign (lhs, expr, &pre);
>>>>        }
>>>>      else
>>>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>>> +         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>> 
>>>>      input_location = saved_location;
>>>>      pop_gimplify_context (NULL);
>>> 
>>> OK, thanks for the explanation.  Unfortunately this fails bootstrap with a failed
>>> assert a little later.  I'll dig further.
>> 
>> Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue
>> passes.  Trying this now:
> 
> Hmm, it should simply gimplify to a load if it's not aggregate.  Can
> you share a testcase
> where it doesn't work?

Your suggestion failed bootstrap in libiberty on vprintf-support.c.  Compilation failed with:

/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/ -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include    -c -DHAVE_CONFIG_H -g -O2 -gtoggle  -I. -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include  -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic  -D_GNU_SOURCE -fPIC /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o pic/vprintf-support.o

The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)).  Gimplification
turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test fails on that.

Bill

> 
>> Index: gcc/tree-stdarg.c
>> ===================================================================
>> --- gcc/tree-stdarg.c   (revision 246109)
>> +++ gcc/tree-stdarg.c   (working copy)
>> @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun)
>>               types.  */
>>            gimplify_assign (lhs, expr, &pre);
>>          }
>> +       else if (is_gimple_addressable (expr))
> 
> I believe any is_gimple_addressable check is flawed.

OK, just wanted to try something quick and dirty before your end of day.  Not sure how
else to deal with this.

Bill

> 
>> +         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>        else
>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>> +         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue);
>> 
>>        input_location = saved_location;
>>        pop_gimplify_context (NULL);
>> 
>> 
>>> 
>>> Bill
Bill Schmidt March 14, 2017, 4:07 p.m. UTC | #3 
On Mar 14, 2017, at 9:32 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> 
>> On Mar 14, 2017, at 9:25 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> 
>>>>> Better is sth like
>>>>> 
>>>>> Index: gcc/tree-stdarg.c
>>>>> ===================================================================
>>>>> --- gcc/tree-stdarg.c   (revision 246082)
>>>>> +++ gcc/tree-stdarg.c   (working copy)
>>>>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun)
>>>>>         gimplify_assign (lhs, expr, &pre);
>>>>>       }
>>>>>     else
>>>>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>>>> +         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>>> 
>>>>>     input_location = saved_location;
>>>>>     pop_gimplify_context (NULL);
>>>> 
>>>> OK, thanks for the explanation.  Unfortunately this fails bootstrap with a failed
>>>> assert a little later.  I'll dig further.
>>> 
>>> Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue
>>> passes.  Trying this now:
>> 
>> Hmm, it should simply gimplify to a load if it's not aggregate.  Can
>> you share a testcase
>> where it doesn't work?
> 
> Your suggestion failed bootstrap in libiberty on vprintf-support.c.  Compilation failed with:
> 
> /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/ -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include    -c -DHAVE_CONFIG_H -g -O2 -gtoggle  -I. -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include  -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic  -D_GNU_SOURCE -fPIC /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o pic/vprintf-support.o
> 
> The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)).  Gimplification
> turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test fails on that.

Reduced test case:

typedef __builtin_va_list __gnuc_va_list;
typedef __gnuc_va_list va_list;

void
foo (va_list args)
{
  va_list ap;
  __builtin_va_copy (ap, args);
  (void)__builtin_va_arg (ap, int);
  __builtin_va_end(ap);
}
diff mbox

Patch

Index: gcc/tree-stdarg.c                                                        
===================================================================
--- gcc/tree-stdarg.c   (revision 246109)                                       
+++ gcc/tree-stdarg.c   (working copy)                                          
@@ -1057,8 +1057,10 @@  expand_ifn_va_arg_1 (function *fun)
               types.  */                                                       
            gimplify_assign (lhs, expr, &pre);                                  
          }                                                                     
+       else if (is_gimple_addressable (expr))                                  
+         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);      
        else                                                                    
-         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);      
+         gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue);         
                                                                                
        input_location = saved_location;                                        
        pop_gimplify_context (NULL);