Re: [PATCH] Fix PR79908

[Bill Schmidt]

On Mar 20, 2017, at 3:26 AM, Richard Biener  wrote:
> 
> Hmm, I think force_gimple_oeprand overwrites what is in  so can
> you try with using a temporary sequence for force_gimple_operand and
> appending that to pre afterwards instead?

Indeed, that solves the problem.  I'll prepare the patch for
review.  Thanks!

Bill

Re: [PATCH] Fix PR79908

[Richard Biener]

On Fri, Mar 17, 2017 at 6:27 PM, Bill Schmidt
 wrote:
>
>> On Mar 17, 2017, at 9:52 AM, Bill Schmidt  
>> wrote:
>>
>> Hi,
>>
>>> On Mar 17, 2017, at 6:44 AM, Richard Biener  
>>> wrote:
>>>
>>> No, I was confused in thinking gimplify_expr would handle the case
>>> properly.  For
>>> just gimplifying side-effects we should use the middle-end
>>> gimplification machinery:
>>>
>>> Index: tree-stdarg.c
>>> ===
>>> --- tree-stdarg.c   (revision 246188)
>>> +++ tree-stdarg.c   (working copy)
>>> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
>>> #include "gimple-iterator.h"
>>> #include "gimple-walk.h"
>>> #include "gimplify.h"
>>> +#include "gimplify-me.h"
>>> #include "tree-into-ssa.h"
>>> #include "tree-cfg.h"
>>> #include "tree-stdarg.h"
>>> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
>>>   gimplify_assign (lhs, expr, );
>>> }
>>>   else
>>> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
>>> + force_gimple_operand (expr, , false, NULL_TREE);
>>>
>>>   input_location = saved_location;
>>>   pop_gimplify_context (NULL);
>>>
>>> -   gimple_seq_add_seq (, post);
>>> +   gimple_seq_add_seq_without_update (, post);
>>>   update_modified_stmts (pre);
>>>
>>>   /* Add the sequence after IFN_VA_ARG.  This splits the bb right
>>> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
>>>   gimple_find_sub_bbs (pre, );
>>>
>>>   /* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
>>> -  bb.  */
>>> +  bb if we added any stmts.  */
>>>   unlink_stmt_vdef (stmt);
>>>   release_ssa_name_fn (fun, gimple_vdef (stmt));
>>>   gsi_remove (, true);
>>> -   gcc_assert (gsi_end_p (i));
>>>
>>>   /* We're walking here into the bbs which contain the expansion of
>>>  IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs
>>
>> Looks good, but for some reason I hit a segfault in the linker building
>> gengtype when I tried to bootstrap with this.  I assume it's something
>> latent and unrelated, but will need to investigate.
>
> Ah, I misread the build log.  The link of gengtype succeeds, but gengtype
> has apparently been miscompiled (stage2) as it tries to call strlen() with
> an address of zero.  So something wrong with this patch.  Looks like the
> miscompilation is of libiberty_vprintf_buffer_size in 
> libiberty/vprintf-support.c.
>
> I looked at the before and after dumps and we see that VA_ARGs with
> no lhs are just removed from the code, instead of expanding the advance
> of the va pointer.
>
> Before:
>
>  [1.39%]:
>   VA_ARG (, 0B, 0B);
>   goto  (); [100.00%]
>
>  [1.39%]:
>   VA_ARG (, 0B, 0B);
>   total_width_89 = total_width_17 + 337;
>   goto  (); [100.00%]
>
> After:
>
>  [1.39%]:
>   goto  (); [100.00%]
>
>  [1.39%]:
>   total_width_89 = total_width_17 + 337;
>   goto  (); [100.00%]

Hmm, I think force_gimple_oeprand overwrites what is in  so can
you try with using a temporary sequence for force_gimple_operand and
appending that to pre afterwards instead?

> Thanks,
> Bill

Re: [PATCH] Fix PR79908

[Bill Schmidt]

> On Mar 17, 2017, at 9:52 AM, Bill Schmidt  wrote:
> 
> Hi,
> 
>> On Mar 17, 2017, at 6:44 AM, Richard Biener  
>> wrote:
>> 
>> No, I was confused in thinking gimplify_expr would handle the case
>> properly.  For
>> just gimplifying side-effects we should use the middle-end
>> gimplification machinery:
>> 
>> Index: tree-stdarg.c
>> ===
>> --- tree-stdarg.c   (revision 246188)
>> +++ tree-stdarg.c   (working copy)
>> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
>> #include "gimple-iterator.h"
>> #include "gimple-walk.h"
>> #include "gimplify.h"
>> +#include "gimplify-me.h"
>> #include "tree-into-ssa.h"
>> #include "tree-cfg.h"
>> #include "tree-stdarg.h"
>> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
>>   gimplify_assign (lhs, expr, );
>> }
>>   else
>> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
>> + force_gimple_operand (expr, , false, NULL_TREE);
>> 
>>   input_location = saved_location;
>>   pop_gimplify_context (NULL);
>> 
>> -   gimple_seq_add_seq (, post);
>> +   gimple_seq_add_seq_without_update (, post);
>>   update_modified_stmts (pre);
>> 
>>   /* Add the sequence after IFN_VA_ARG.  This splits the bb right
>> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
>>   gimple_find_sub_bbs (pre, );
>> 
>>   /* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
>> -  bb.  */
>> +  bb if we added any stmts.  */
>>   unlink_stmt_vdef (stmt);
>>   release_ssa_name_fn (fun, gimple_vdef (stmt));
>>   gsi_remove (, true);
>> -   gcc_assert (gsi_end_p (i));
>> 
>>   /* We're walking here into the bbs which contain the expansion of
>>  IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs
> 
> Looks good, but for some reason I hit a segfault in the linker building 
> gengtype when I tried to bootstrap with this.  I assume it's something
> latent and unrelated, but will need to investigate.

Ah, I misread the build log.  The link of gengtype succeeds, but gengtype
has apparently been miscompiled (stage2) as it tries to call strlen() with
an address of zero.  So something wrong with this patch.  Looks like the
miscompilation is of libiberty_vprintf_buffer_size in 
libiberty/vprintf-support.c.

I looked at the before and after dumps and we see that VA_ARGs with
no lhs are just removed from the code, instead of expanding the advance
of the va pointer.

Before:

 [1.39%]:  
  VA_ARG (, 0B, 0B); 
  goto  (); [100.00%]   

 [1.39%]:  
  VA_ARG (, 0B, 0B); 
  total_width_89 = total_width_17 + 337;
  goto  (); [100.00%]   

After:

 [1.39%]:
  goto  (); [100.00%]

 [1.39%]:
  total_width_89 = total_width_17 + 337;
  goto  (); [100.00%]

Thanks,
Bill

Re: [PATCH] Fix PR79908

[Bill Schmidt]

Hi,

> On Mar 17, 2017, at 6:44 AM, Richard Biener  
> wrote:
> 
> No, I was confused in thinking gimplify_expr would handle the case
> properly.  For
> just gimplifying side-effects we should use the middle-end
> gimplification machinery:
> 
> Index: tree-stdarg.c
> ===
> --- tree-stdarg.c   (revision 246188)
> +++ tree-stdarg.c   (working copy)
> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
> #include "gimple-iterator.h"
> #include "gimple-walk.h"
> #include "gimplify.h"
> +#include "gimplify-me.h"
> #include "tree-into-ssa.h"
> #include "tree-cfg.h"
> #include "tree-stdarg.h"
> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
>gimplify_assign (lhs, expr, );
>  }
>else
> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
> + force_gimple_operand (expr, , false, NULL_TREE);
> 
>input_location = saved_location;
>pop_gimplify_context (NULL);
> 
> -   gimple_seq_add_seq (, post);
> +   gimple_seq_add_seq_without_update (, post);
>update_modified_stmts (pre);
> 
>/* Add the sequence after IFN_VA_ARG.  This splits the bb right
> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
>gimple_find_sub_bbs (pre, );
> 
>/* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
> -  bb.  */
> +  bb if we added any stmts.  */
>unlink_stmt_vdef (stmt);
>release_ssa_name_fn (fun, gimple_vdef (stmt));
>gsi_remove (, true);
> -   gcc_assert (gsi_end_p (i));
> 
>/* We're walking here into the bbs which contain the expansion of
>   IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs

Looks good, but for some reason I hit a segfault in the linker building 
gengtype when I tried to bootstrap with this.  I assume it's something
latent and unrelated, but will need to investigate.

Bill

Re: [PATCH] Fix PR79908

[Richard Biener]

On Tue, Mar 14, 2017 at 10:36 PM, Bill Schmidt
 wrote:
>
>> On Mar 14, 2017, at 11:07 AM, Bill Schmidt  
>> wrote:
>>>
>>> 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);
>> }
>>
>
> Perhaps something like this?  It appears to be doing better on bootstrap, and 
> avoids
> failure on both the original problem and the new test case above:
>
> Index: gcc/tree-stdarg.c
> ===
> --- gcc/tree-stdarg.c   (revision 246109)
> +++ gcc/tree-stdarg.c   (working copy)
> @@ -1058,7 +1058,13 @@ expand_ifn_va_arg_1 (function *fun)
> gimplify_assign (lhs, expr, );
>   }
> else
> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
> + {
> +   enum gimplify_status status;
> +   status = gimplify_expr (, , , is_gimple_lvalue,
> +   fb_lvalue | fb_mayfail);
> +   if (status == GS_ERROR)
> + gimplify_expr (, , , is_gimple_val, fb_rvalue);
> + }
>
> input_location = saved_location;
> pop_gimplify_context (NULL);

No, I was confused in thinking gimplify_expr would handle the case
properly.  For
just gimplifying side-effects we should use the middle-end
gimplification machinery:

Index: tree-stdarg.c
===
--- tree-stdarg.c   (revision 246188)
+++ tree-stdarg.c   (working copy)
@@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
 #include "gimple-iterator.h"
 #include "gimple-walk.h"
 #include "gimplify.h"
+#include "gimplify-me.h"
 #include "tree-into-ssa.h"
 #include "tree-cfg.h"
 #include "tree-stdarg.h"
@@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
gimplify_assign (lhs, expr, );
  }
else
- gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
+ force_gimple_operand (expr, , false, NULL_TREE);

input_location = saved_location;
pop_gimplify_context (NULL);

-   gimple_seq_add_seq (, post);
+   gimple_seq_add_seq_without_update (, post);
update_modified_stmts (pre);

/* Add the sequence after IFN_VA_ARG.  This splits the bb right
@@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
gimple_find_sub_bbs (pre, );

/* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
-  bb.  */
+  bb if we added any stmts.  */
unlink_stmt_vdef (stmt);
release_ssa_name_fn (fun, gimple_vdef (stmt));
gsi_remove (, true);
-   gcc_assert (gsi_end_p (i));

/* We're walking here into the bbs which contain the expansion of
   IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs


> Bill

Re: [PATCH] Fix PR79908

[Bill Schmidt]

> On Mar 14, 2017, at 9:25 AM, Richard Biener  
> wrote:
> 
> On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt
>  wrote:
>> 
>>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt  
>>> wrote:
>>> 
>>> 
 On Mar 14, 2017, at 3:57 AM, Richard Biener  
 wrote:
 
 On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
  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, );
>}
> -   else
> +   else if (is_gimple_addressable (expr))
>gimplify_expr (, , , 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, );
}
  else
 - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
 + gimplify_expr (, , , 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?

So taking a step back...

The original problem is that most VA_ARGs get gimplified to a single SSA var,
which always passes the is_gimple_addressable check, so if the LHS has either
been cast away or gone dead, we get a nice lvalue to work with and everyone is
happy.

For a VA_ARG of _Complex type, gimplification instead produces a
COMPLEX_EXPR  where a and b have been reduced to SSA vars.
The is_gimple_addressable check does not pass in this case, so we can't
find the mandated lvalue and ICE in the gimplifier after doing all the work.

I suppose we could paper over this with:

Index: gcc/tree-stdarg.c
===
--- gcc/tree-stdarg.c   (revision 246109)   
+++ gcc/tree-stdarg.c   (working copy)  
@@ -1058,7 +1058,10 @@ expand_ifn_va_arg_1 (function *fun)
gimplify_assign (lhs, expr, );  
  } 
else
- gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);  
+ gimplify_expr (, , , is_gimple_lvalue,  
+fb_lvalue | fb_mayfail);   

...but that is perhaps opening the gate too wide (though we could throw
an error if gimplify_expr fails and expr is not a COMPLEX_EXPR).  The 
COMPLEX_EXPR really is not addressable, but forcing any COMPLEX_EXPR
to a variable in gimplify_expr also seems like overkill.  (All of the tcc_binary
expressions are treated the same; we would fail if somebody asked for an
lvalue of a PLUS_EXPR just the same.)  So offhand I don't see anything
better than the above.

Thoughts?

Thanks,
Bill 

Re: [PATCH] Fix PR79908

[Bill Schmidt]

> On Mar 14, 2017, at 11:07 AM, Bill Schmidt  
> wrote:
>> 
>> 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);
> }
> 

Perhaps something like this?  It appears to be doing better on bootstrap, and 
avoids
failure on both the original problem and the new test case above:

Index: gcc/tree-stdarg.c
===
--- gcc/tree-stdarg.c   (revision 246109)   
+++ gcc/tree-stdarg.c   (working copy)  
@@ -1058,7 +1058,13 @@ expand_ifn_va_arg_1 (function *fun)
gimplify_assign (lhs, expr, );  
  } 
else
- gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);  
+ { 
+   enum gimplify_status status;
+   status = gimplify_expr (, , , is_gimple_lvalue,   
+   fb_lvalue | fb_mayfail);
+   if (status == GS_ERROR) 
+ gimplify_expr (, , , is_gimple_val, fb_rvalue); 
+ } 

input_location = saved_location;
pop_gimplify_context (NULL);

Bill

Re: [PATCH] Fix PR79908

[Bill Schmidt]

On Mar 14, 2017, at 9:32 AM, Bill Schmidt  wrote:
> 
>> On Mar 14, 2017, at 9:25 AM, Richard Biener  
>> 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, );
>   }
> else
> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
> + gimplify_expr (, , , 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);
}

Re: [PATCH] Fix PR79908

[Bill Schmidt]

> On Mar 14, 2017, at 9:25 AM, Richard Biener  
> wrote:
> 
> On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt
>  wrote:
>> 
>>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt  
>>> wrote:
>>> 
>>> 
 On Mar 14, 2017, at 3:57 AM, Richard Biener  
 wrote:
 
 On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
  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, );
>}
> -   else
> +   else if (is_gimple_addressable (expr))
>gimplify_expr (, , , 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, );
}
  else
 - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
 + gimplify_expr (, , , 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, );
>>  }
>> +   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 (, , , is_gimple_lvalue, fb_lvalue);
>>else
>> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
>> + gimplify_expr (, , , is_gimple_val, fb_rvalue);
>> 
>>input_location = saved_location;
>>pop_gimplify_context (NULL);
>> 
>> 
>>> 
>>> Bill

Re: [PATCH] Fix PR79908

[Richard Biener]

On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt
 wrote:
>
>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt  
>> wrote:
>>
>>
>>> On Mar 14, 2017, at 3:57 AM, Richard Biener  
>>> wrote:
>>>
>>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
>>>  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, );
 }
 -   else
 +   else if (is_gimple_addressable (expr))
 gimplify_expr (, , , 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, );
>>> }
>>>   else
>>> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
>>> + gimplify_expr (, , , 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, );
>   }
> +   else if (is_gimple_addressable (expr))

I believe any is_gimple_addressable check is flawed.

> + gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
> else
> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
> + gimplify_expr (, , , is_gimple_val, fb_rvalue);
>
> input_location = saved_location;
> pop_gimplify_context (NULL);
>
>
>>
>> Bill
>>
>

Re: [PATCH] Fix PR79908

[Bill Schmidt]

> On Mar 14, 2017, at 7:50 AM, Bill Schmidt  wrote:
> 
> 
>> On Mar 14, 2017, at 3:57 AM, Richard Biener  
>> wrote:
>> 
>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
>>  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, );
>>> }
>>> -   else
>>> +   else if (is_gimple_addressable (expr))
>>> gimplify_expr (, , , 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, );
>> }
>>   else
>> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
>> + gimplify_expr (, , , 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:

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, );  
  } 
+   else if (is_gimple_addressable (expr))  
+ gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);  
else
- gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);  
+ gimplify_expr (, , , is_gimple_val, fb_rvalue); 

input_location = saved_location;
pop_gimplify_context (NULL);


> 
> Bill
> 

Re: [PATCH] Fix PR79908

[Bill Schmidt]

> On Mar 14, 2017, at 3:57 AM, Richard Biener  
> wrote:
> 
> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
>  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, );
>>  }
>> -   else
>> +   else if (is_gimple_addressable (expr))
>>  gimplify_expr (, , , 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, );
>  }
>else
> - gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
> + gimplify_expr (, , , 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.

Bill

Re: [PATCH] Fix PR79908

[Richard Biener]

On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt
 wrote:
> Hi,
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79908 shows a case where
> pass_stdarg ICEs attempting to gimplify a COMPLEX_EXPR with side
> effects as an lvalue.  The expression is not addressable, so the
> gimplification fails.  This patch says, hey, don't do that!
>
> The resulting GIMPLE looks fine afterward.  Bootstrapped and tested
> on powerpc64le-unknown-linux-gnu with no regressions.  Is this ok
> for trunk?
>
> Thanks,
> Bill
>
>
> [gcc]
>
> 2017-03-13  Bill Schmidt  
>
> PR tree-optimization/79908
> * tree-stdarg.c (expand_ifn_va_arg_1): Don't force something to be
> an lvalue that isn't addressable.
>
> [gcc/testsuite]
>
> 2017-03-13  Bill Schmidt  
>
> PR tree-optimization/79908
> * gcc.dg/torture/pr79908.c: New file.
>
>
> Index: gcc/testsuite/gcc.dg/torture/pr79908.c
> ===
> --- gcc/testsuite/gcc.dg/torture/pr79908.c  (nonexistent)
> +++ gcc/testsuite/gcc.dg/torture/pr79908.c  (working copy)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +
> +/* Used to fail in the stdarg pass before fix for PR79908.  */
> +
> +typedef __builtin_va_list __gnuc_va_list;
> +typedef __gnuc_va_list va_list;
> +
> +void testva (int n, ...)
> +{
> +  va_list ap;
> +  _Complex int i = __builtin_va_arg (ap, _Complex int);
> +}
> 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, );
>   }
> -   else
> +   else if (is_gimple_addressable (expr))
>   gimplify_expr (, , , 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, );
  }
else
- gimplify_expr (, , , is_gimple_lvalue, fb_lvalue);
+ gimplify_expr (, , , is_gimple_val, fb_either);

input_location = saved_location;
pop_gimplify_context (NULL);


>
> input_location = saved_location;
>