* [PATCH] Fix PR71104 - call gimplification
@ 2016-05-17 12:28 Richard Biener
2016-05-18 17:43 ` Jeff Law
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2016-05-17 12:28 UTC (permalink / raw)
To: gcc-patches; +Cc: Joseph S. Myers
The following patch addresses PR71104 which shows verify-SSA ICEs
after gimplify-into-SSA. The issue is that for returns-twice calls
we gimplify register uses in the LHS before the actual call which leads to
p.0_1 = p;
_2 = vfork ();
*p.0_1 = _2;
when gimplifying *p = vfork (). That of course does not work -
fortunately the C standard allows to evaluate operands in the LHS
in unspecified order of the RHS. That also makes this order aligned
with that scary C++ proposal of defined evaluation order. It also
improves code-generation, avoiding spilling of the pointer load
around the call.
Exchanging the gimplify calls doesn't fix the issue fully as for
aggregate returns we don't gimplify the call result into a
temporary. So we need to make sure to not emit an SSA when
gimplifying the LHS of a returns-twice call (this path only applies
to non-register returns).
A bootstrap with just the gimplification order exchange is building
target libs right now, I'll re-bootstrap and test the whole thing
again if that succeeds.
Is this ok? I think it makes sense code-generation-wise. Code
changes from GCC 6
bar:
.LFB0:
.cfi_startproc
subq $24, %rsp
.cfi_def_cfa_offset 32
call foo
movq p(%rip), %rax
movq %rax, 8(%rsp)
call vfork
movq 8(%rsp), %rdx
movl %eax, (%rdx)
addq $24, %rsp
.cfi_def_cfa_offset 8
ret
to
bar:
.LFB0:
.cfi_startproc
subq $8, %rsp
.cfi_def_cfa_offset 16
call foo
call vfork
movq p(%rip), %rdx
movl %eax, (%rdx)
addq $8, %rsp
.cfi_def_cfa_offset 8
ret
Thanks,
Richard.
2016-05-17 Richard Biener <rguenther@suse.de>
PR middle-end/71104
* gimplify.c (gimplify_modify_expr): Gimplify the RHS before
gimplifying the LHS. Make sure to gimplify a returning twice
call LHS without using SSA names.
* gcc.dg/pr71104-1.c: New testcase.
* gcc.dg/pr71104-2.c: Likewise.
Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c (revision 236317)
--- gcc/gimplify.c (working copy)
*************** gimplify_modify_expr (tree *expr_p, gimp
*** 4708,4717 ****
that is what we must do here. */
maybe_with_size_expr (from_p);
- ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
- if (ret == GS_ERROR)
- return ret;
-
/* As a special case, we have to temporarily allow for assignments
with a CALL_EXPR on the RHS. Since in GIMPLE a function call is
a toplevel statement, when gimplifying the GENERIC expression
--- 4708,4713 ----
*************** gimplify_modify_expr (tree *expr_p, gimp
*** 4729,4734 ****
--- 4725,4746 ----
if (ret == GS_ERROR)
return ret;
+ /* If we gimplified the RHS to a CALL_EXPR and that call may return
+ twice we have to make sure to gimplify into non-SSA as otherwise
+ the abnormal edge added later will make those defs not dominate
+ their uses.
+ ??? Technically this applies only to the registers used in the
+ resulting non-register *TO_P. */
+ bool saved_into_ssa = gimplify_ctxp->into_ssa;
+ if (saved_into_ssa
+ && TREE_CODE (*from_p) == CALL_EXPR
+ && call_expr_flags (*from_p) & ECF_RETURNS_TWICE)
+ gimplify_ctxp->into_ssa = false;
+ ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
+ gimplify_ctxp->into_ssa = saved_into_ssa;
+ if (ret == GS_ERROR)
+ return ret;
+
/* In case of va_arg internal fn wrappped in a WITH_SIZE_EXPR, add the type
size as argument to the call. */
if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)
Index: gcc/testsuite/gcc.dg/pr71104-1.c
===================================================================
*** gcc/testsuite/gcc.dg/pr71104-1.c (revision 0)
--- gcc/testsuite/gcc.dg/pr71104-1.c (working copy)
***************
*** 0 ****
--- 1,11 ----
+ /* { dg-do compile } */
+
+ void foo(void);
+ int vfork(void);
+ int *p;
+
+ void bar(void)
+ {
+ foo();
+ *p = vfork();
+ }
Index: gcc/testsuite/gcc.dg/pr71104-2.c
===================================================================
*** gcc/testsuite/gcc.dg/pr71104-2.c (revision 0)
--- gcc/testsuite/gcc.dg/pr71104-2.c (working copy)
***************
*** 0 ****
--- 1,12 ----
+ /* { dg-do compile } */
+
+ struct Foo { char c[1024]; };
+ void foo(void);
+ struct Foo baz(void) __attribute__((returns_twice));
+ struct Foo *p;
+
+ void bar(void)
+ {
+ foo();
+ *p = baz();
+ }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix PR71104 - call gimplification
2016-05-17 12:28 [PATCH] Fix PR71104 - call gimplification Richard Biener
@ 2016-05-18 17:43 ` Jeff Law
2016-07-13 7:59 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Law @ 2016-05-18 17:43 UTC (permalink / raw)
To: Richard Biener, gcc-patches; +Cc: Joseph S. Myers
On 05/17/2016 06:28 AM, Richard Biener wrote:
>
> The following patch addresses PR71104 which shows verify-SSA ICEs
> after gimplify-into-SSA. The issue is that for returns-twice calls
> we gimplify register uses in the LHS before the actual call which leads to
>
> p.0_1 = p;
> _2 = vfork ();
> *p.0_1 = _2;
>
> when gimplifying *p = vfork (). That of course does not work -
> fortunately the C standard allows to evaluate operands in the LHS
> in unspecified order of the RHS. That also makes this order aligned
> with that scary C++ proposal of defined evaluation order. It also
> improves code-generation, avoiding spilling of the pointer load
> around the call.
>
> Exchanging the gimplify calls doesn't fix the issue fully as for
> aggregate returns we don't gimplify the call result into a
> temporary. So we need to make sure to not emit an SSA when
> gimplifying the LHS of a returns-twice call (this path only applies
> to non-register returns).
>
> A bootstrap with just the gimplification order exchange is building
> target libs right now, I'll re-bootstrap and test the whole thing
> again if that succeeds.
>
> Is this ok? I think it makes sense code-generation-wise. Code
> changes from GCC 6
>
> bar:
> .LFB0:
> .cfi_startproc
> subq $24, %rsp
> .cfi_def_cfa_offset 32
> call foo
> movq p(%rip), %rax
> movq %rax, 8(%rsp)
> call vfork
> movq 8(%rsp), %rdx
> movl %eax, (%rdx)
> addq $24, %rsp
> .cfi_def_cfa_offset 8
> ret
>
> to
>
> bar:
> .LFB0:
> .cfi_startproc
> subq $8, %rsp
> .cfi_def_cfa_offset 16
> call foo
> call vfork
> movq p(%rip), %rdx
> movl %eax, (%rdx)
> addq $8, %rsp
> .cfi_def_cfa_offset 8
> ret
>
> Thanks,
> Richard.
>
> 2016-05-17 Richard Biener <rguenther@suse.de>
>
> PR middle-end/71104
> * gimplify.c (gimplify_modify_expr): Gimplify the RHS before
> gimplifying the LHS. Make sure to gimplify a returning twice
> call LHS without using SSA names.
>
> * gcc.dg/pr71104-1.c: New testcase.
> * gcc.dg/pr71104-2.c: Likewise.
LGTM.
jeff
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix PR71104 - call gimplification
2016-05-18 17:43 ` Jeff Law
@ 2016-07-13 7:59 ` Richard Biener
0 siblings, 0 replies; 3+ messages in thread
From: Richard Biener @ 2016-07-13 7:59 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches, Joseph S. Myers
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
On Wed, May 18, 2016 at 7:43 PM, Jeff Law <law@redhat.com> wrote:
> On 05/17/2016 06:28 AM, Richard Biener wrote:
>>
>>
>> The following patch addresses PR71104 which shows verify-SSA ICEs
>> after gimplify-into-SSA. The issue is that for returns-twice calls
>> we gimplify register uses in the LHS before the actual call which leads to
>>
>> p.0_1 = p;
>> _2 = vfork ();
>> *p.0_1 = _2;
>>
>> when gimplifying *p = vfork (). That of course does not work -
>> fortunately the C standard allows to evaluate operands in the LHS
>> in unspecified order of the RHS. That also makes this order aligned
>> with that scary C++ proposal of defined evaluation order. It also
>> improves code-generation, avoiding spilling of the pointer load
>> around the call.
>>
>> Exchanging the gimplify calls doesn't fix the issue fully as for
>> aggregate returns we don't gimplify the call result into a
>> temporary. So we need to make sure to not emit an SSA when
>> gimplifying the LHS of a returns-twice call (this path only applies
>> to non-register returns).
>>
>> A bootstrap with just the gimplification order exchange is building
>> target libs right now, I'll re-bootstrap and test the whole thing
>> again if that succeeds.
>>
>> Is this ok? I think it makes sense code-generation-wise. Code
>> changes from GCC 6
>>
>> bar:
>> .LFB0:
>> .cfi_startproc
>> subq $24, %rsp
>> .cfi_def_cfa_offset 32
>> call foo
>> movq p(%rip), %rax
>> movq %rax, 8(%rsp)
>> call vfork
>> movq 8(%rsp), %rdx
>> movl %eax, (%rdx)
>> addq $24, %rsp
>> .cfi_def_cfa_offset 8
>> ret
>>
>> to
>>
>> bar:
>> .LFB0:
>> .cfi_startproc
>> subq $8, %rsp
>> .cfi_def_cfa_offset 16
>> call foo
>> call vfork
>> movq p(%rip), %rdx
>> movl %eax, (%rdx)
>> addq $8, %rsp
>> .cfi_def_cfa_offset 8
>> ret
>>
>> Thanks,
>> Richard.
>>
>> 2016-05-17 Richard Biener <rguenther@suse.de>
>>
>> PR middle-end/71104
>> * gimplify.c (gimplify_modify_expr): Gimplify the RHS before
>> gimplifying the LHS. Make sure to gimplify a returning twice
>> call LHS without using SSA names.
>>
>> * gcc.dg/pr71104-1.c: New testcase.
>> * gcc.dg/pr71104-2.c: Likewise.
>
> LGTM.
Now after Jason applied a better solution for gimplifying the RHS
before the LHS I have
applied the following after bootstrap / regtest on x86_64-unknown-linux-gnu.
Richard.
> jeff
>
[-- Attachment #2: fix-pr71104 --]
[-- Type: application/octet-stream, Size: 2109 bytes --]
2016-07-13 Richard Biener <rguenther@suse.de>
PR middle-end/71104
* gimplify.c (gimplify_modify_expr): Gimplify the RHS before
gimplifying the LHS. Make sure to gimplify a returning twice
call LHS without using SSA names.
* gcc.dg/pr71104-1.c: New testcase.
* gcc.dg/pr71104-2.c: Likewise.
Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c (revision 238237)
--- gcc/gimplify.c (working copy)
*************** gimplify_modify_expr (tree *expr_p, gimp
*** 4810,4816 ****
--- 4812,4830 ----
return ret;
/* Then gimplify the LHS. */
+ /* If we gimplified the RHS to a CALL_EXPR and that call may return
+ twice we have to make sure to gimplify into non-SSA as otherwise
+ the abnormal edge added later will make those defs not dominate
+ their uses.
+ ??? Technically this applies only to the registers used in the
+ resulting non-register *TO_P. */
+ bool saved_into_ssa = gimplify_ctxp->into_ssa;
+ if (saved_into_ssa
+ && TREE_CODE (*from_p) == CALL_EXPR
+ && call_expr_flags (*from_p) & ECF_RETURNS_TWICE)
+ gimplify_ctxp->into_ssa = false;
ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
+ gimplify_ctxp->into_ssa = saved_into_ssa;
if (ret == GS_ERROR)
return ret;
Index: gcc/testsuite/gcc.dg/pr71104-1.c
===================================================================
--- gcc/testsuite/gcc.dg/pr71104-1.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr71104-1.c (revision 0)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+
+void foo(void);
+int vfork(void);
+int *p;
+
+void bar(void)
+{
+ foo();
+ *p = vfork();
+}
Index: gcc/testsuite/gcc.dg/pr71104-2.c
===================================================================
--- gcc/testsuite/gcc.dg/pr71104-2.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr71104-2.c (revision 0)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+
+struct Foo { char c[1024]; };
+void foo(void);
+struct Foo baz(void) __attribute__((returns_twice));
+struct Foo *p;
+
+void bar(void)
+{
+ foo();
+ *p = baz();
+}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-13 7:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-17 12:28 [PATCH] Fix PR71104 - call gimplification Richard Biener
2016-05-18 17:43 ` Jeff Law
2016-07-13 7:59 ` Richard Biener
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).