Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

On Aug 1, 2017, at 10:56 AM, Bill Schmidt  wrote:
> 
> On Aug 1, 2017, at 8:50 AM, Bill Schmidt  wrote:
>> 
>> On Aug 1, 2017, at 7:44 AM, Bill Schmidt  wrote:
>>> 
 
 On Aug 1, 2017, at 3:46 AM, Richard Biener  
 wrote:
 
 On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
  wrote:
> 
>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
>> wrote:
>> 
>> That would certainly be much simpler!  I'll regstrap it and test it on 
>> the other
>> occurrence I've found to be certain.
> 
> Unfortunately, this fails bootstrap:
> 
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
> va_list)':
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: 
> definition in block 214 does not dominate use in block 14
> emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
> ^
> for SSA_NAME: slsr_772 in statement:
> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
> PHI argument
> slsr_772
> for PHI node
> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
> during GIMPLE pass: slsr
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal 
> compiler error: verify_ssa failed
> 0x11567cf3 verify_ssa(bool, bool)
> /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
> 0x10fc3fff execute_function_todo
> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
> 0x10fc277f do_per_function
> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
> 0x10fc42a3 execute_todo
> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See  for instructions.
> 
> Not terribly surprising given how sensitive this stuff is.  I can look 
> into why
> this fails, but looks like it can't be quite this simple, sadly.
 
 Intersting ... a dg-torture.exp run was clean for me (all I
 tested...).  So yes, can you
 check what fails?  Maybe run the testsuite with the stage1 compiler.
>>> 
>>> Looks like it's the stage1 that doesn't build.  I think the difference is
>>> that I was building trunk and you were building 6.  I'll try to look into
>>> it later today after I get through some meetings.
>> 
>> Sorry, no, it was stage 2 where the failure occurs.
> 
> Ah, "good" news.  I believe the bootstrap failure with this change is another
> rediscovery of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81503, which
> explains why it wasn't seen for GCC 6.  I'll work on getting that fixed and
> then try this again.

Well, no.  I added the fix for 81503 to your patch, and I still get a failure on
trunk.  For the time being I am going to look at the edge-insertion idea 
before spending any more time on gimple_split_edge.

Bill

> 
> Bill
> 
>> 
>> Bill
>> 
>>> 
>>> Bill
 
 Richard.
 
> Bill
> 
>> 
>> -- Bill
>> 
>>> On Jul 31, 2017, at 4:15 AM, Richard Biener 
>>>  wrote:
>>> 
>>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>>>  wrote:
 Hi,
 
 PR81354 identifies a latent bug that can happen in SLSR since the
 conditional candidate support was first added.  SLSR relies on the
 address of a GIMPLE PHI remaining constant during the course of the
 optimization pass, but it needs to split edges.  The use of
 make_single_succ_edge and reinstall_phi_args in gimple_split_edge
 causes GIMPLE PHI statements to be temporarily expanded to add a
 predecessor, and then rebuilt to have the original number of
 predecessors.  The expansion usually, if not always, causes the PHI
 statement to change address.  Thus gimple_split_edge needs to be
 rewritten to perform in-situ replacement of PHI arguments.
 
 The required pieces of make_single_succ_edge have been extracted into
 two places:  make_replacement_pred_edge, and some fixup code at the
 end of gimple_split_edge.  The division is necessary because the
 destination of the original edge must remember its original
 predecessors for the switch processing in
 gimple_redirect_edge_and_branch_1 to work properly.
 
 The function gimple_redirect_edge_and_branch was factored into two
 pieces so that most of it can be used by gimple_split_edge without
 calling ssa_redirect_edge, which also interferes with PHIs.  The
 useful bits of 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

On Aug 2, 2017, at 3:09 AM, Richard Biener  wrote:
> 
> On Tue, Aug 1, 2017 at 5:56 PM, Bill Schmidt
>  wrote:
>> On Aug 1, 2017, at 8:50 AM, Bill Schmidt  wrote:
>>> 
>>> On Aug 1, 2017, at 7:44 AM, Bill Schmidt  
>>> wrote:
 
> 
> On Aug 1, 2017, at 3:46 AM, Richard Biener  
> wrote:
> 
> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
>  wrote:
>> 
>>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
>>> wrote:
>>> 
>>> That would certainly be much simpler!  I'll regstrap it and test it on 
>>> the other
>>> occurrence I've found to be certain.
>> 
>> Unfortunately, this fails bootstrap:
>> 
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
>> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, 
>> int, va_list)':
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: 
>> definition in block 214 does not dominate use in block 14
>> emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
>> ^
>> for SSA_NAME: slsr_772 in statement:
>> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
>> PHI argument
>> slsr_772
>> for PHI node
>> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
>> during GIMPLE pass: slsr
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal 
>> compiler error: verify_ssa failed
>> 0x11567cf3 verify_ssa(bool, bool)
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
>> 0x10fc3fff execute_function_todo
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
>> 0x10fc277f do_per_function
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
>> 0x10fc42a3 execute_todo
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
>> Please submit a full bug report,
>> with preprocessed source if appropriate.
>> Please include the complete backtrace with any bug report.
>> See  for instructions.
>> 
>> Not terribly surprising given how sensitive this stuff is.  I can look 
>> into why
>> this fails, but looks like it can't be quite this simple, sadly.
> 
> Intersting ... a dg-torture.exp run was clean for me (all I
> tested...).  So yes, can you
> check what fails?  Maybe run the testsuite with the stage1 compiler.
 
 Looks like it's the stage1 that doesn't build.  I think the difference is
 that I was building trunk and you were building 6.  I'll try to look into
 it later today after I get through some meetings.
>>> 
>>> Sorry, no, it was stage 2 where the failure occurs.
>> 
>> Ah, "good" news.  I believe the bootstrap failure with this change is another
>> rediscovery of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81503, which
>> explains why it wasn't seen for GCC 6.  I'll work on getting that fixed and
>> then try this again.
> 
> Note I very much would like to "fix" split_edge on trunk, on release branches
> I'd appreciate if a simpler fix inside SLSR works, like using 
> gsi_insert_on_edge
> to avoid splitting edges during the iteration.

Sure, I'll look into it.

Bill
> 
> Richard.
> 
>> Bill
>> 
>>> 
>>> Bill
>>> 
 
 Bill
> 
> Richard.
> 
>> Bill
>> 
>>> 
>>> -- Bill
>>> 
 On Jul 31, 2017, at 4:15 AM, Richard Biener 
  wrote:
 
 On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
  wrote:
> Hi,
> 
> PR81354 identifies a latent bug that can happen in SLSR since the
> conditional candidate support was first added.  SLSR relies on the
> address of a GIMPLE PHI remaining constant during the course of the
> optimization pass, but it needs to split edges.  The use of
> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
> causes GIMPLE PHI statements to be temporarily expanded to add a
> predecessor, and then rebuilt to have the original number of
> predecessors.  The expansion usually, if not always, causes the PHI
> statement to change address.  Thus gimple_split_edge needs to be
> rewritten to perform in-situ replacement of PHI arguments.
> 
> The required pieces of make_single_succ_edge have been extracted into
> two places:  make_replacement_pred_edge, and some fixup code at the
> end of gimple_split_edge.  The division is necessary because the
> destination of the original edge must remember its original
> predecessors for the switch processing in
> gimple_redirect_edge_and_branch_1 to work properly.
> 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Richard Biener]

On Tue, Aug 1, 2017 at 5:56 PM, Bill Schmidt
 wrote:
> On Aug 1, 2017, at 8:50 AM, Bill Schmidt  wrote:
>>
>> On Aug 1, 2017, at 7:44 AM, Bill Schmidt  wrote:
>>>

 On Aug 1, 2017, at 3:46 AM, Richard Biener  
 wrote:

 On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
  wrote:
>
>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
>> wrote:
>>
>> That would certainly be much simpler!  I'll regstrap it and test it on 
>> the other
>> occurrence I've found to be certain.
>
> Unfortunately, this fails bootstrap:
>
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
> va_list)':
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: 
> definition in block 214 does not dominate use in block 14
> emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
> ^
> for SSA_NAME: slsr_772 in statement:
> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
> PHI argument
> slsr_772
> for PHI node
> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
> during GIMPLE pass: slsr
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal 
> compiler error: verify_ssa failed
> 0x11567cf3 verify_ssa(bool, bool)
>  /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
> 0x10fc3fff execute_function_todo
>  /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
> 0x10fc277f do_per_function
>  /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
> 0x10fc42a3 execute_todo
>  /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See  for instructions.
>
> Not terribly surprising given how sensitive this stuff is.  I can look 
> into why
> this fails, but looks like it can't be quite this simple, sadly.

 Intersting ... a dg-torture.exp run was clean for me (all I
 tested...).  So yes, can you
 check what fails?  Maybe run the testsuite with the stage1 compiler.
>>>
>>> Looks like it's the stage1 that doesn't build.  I think the difference is
>>> that I was building trunk and you were building 6.  I'll try to look into
>>> it later today after I get through some meetings.
>>
>> Sorry, no, it was stage 2 where the failure occurs.
>
> Ah, "good" news.  I believe the bootstrap failure with this change is another
> rediscovery of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81503, which
> explains why it wasn't seen for GCC 6.  I'll work on getting that fixed and
> then try this again.

Note I very much would like to "fix" split_edge on trunk, on release branches
I'd appreciate if a simpler fix inside SLSR works, like using gsi_insert_on_edge
to avoid splitting edges during the iteration.

Richard.

> Bill
>
>>
>> Bill
>>
>>>
>>> Bill

 Richard.

> Bill
>
>>
>> -- Bill
>>
>>> On Jul 31, 2017, at 4:15 AM, Richard Biener 
>>>  wrote:
>>>
>>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>>>  wrote:
 Hi,

 PR81354 identifies a latent bug that can happen in SLSR since the
 conditional candidate support was first added.  SLSR relies on the
 address of a GIMPLE PHI remaining constant during the course of the
 optimization pass, but it needs to split edges.  The use of
 make_single_succ_edge and reinstall_phi_args in gimple_split_edge
 causes GIMPLE PHI statements to be temporarily expanded to add a
 predecessor, and then rebuilt to have the original number of
 predecessors.  The expansion usually, if not always, causes the PHI
 statement to change address.  Thus gimple_split_edge needs to be
 rewritten to perform in-situ replacement of PHI arguments.

 The required pieces of make_single_succ_edge have been extracted into
 two places:  make_replacement_pred_edge, and some fixup code at the
 end of gimple_split_edge.  The division is necessary because the
 destination of the original edge must remember its original
 predecessors for the switch processing in
 gimple_redirect_edge_and_branch_1 to work properly.

 The function gimple_redirect_edge_and_branch was factored into two
 pieces so that most of it can be used by gimple_split_edge without
 calling ssa_redirect_edge, which also interferes with PHIs.  The
 useful bits of ssa_redirect_edge are 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

On Aug 1, 2017, at 8:50 AM, Bill Schmidt  wrote:
> 
> On Aug 1, 2017, at 7:44 AM, Bill Schmidt  wrote:
>> 
>>> 
>>> On Aug 1, 2017, at 3:46 AM, Richard Biener  
>>> wrote:
>>> 
>>> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
>>>  wrote:
 
> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
> wrote:
> 
> That would certainly be much simpler!  I'll regstrap it and test it on 
> the other
> occurrence I've found to be certain.
 
 Unfortunately, this fails bootstrap:
 
 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
 emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
 va_list)':
 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: definition 
 in block 214 does not dominate use in block 14
 emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
 ^
 for SSA_NAME: slsr_772 in statement:
 slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
 PHI argument
 slsr_772
 for PHI node
 slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
 during GIMPLE pass: slsr
 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal compiler 
 error: verify_ssa failed
 0x11567cf3 verify_ssa(bool, bool)
  /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
 0x10fc3fff execute_function_todo
  /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
 0x10fc277f do_per_function
  /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
 0x10fc42a3 execute_todo
  /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
 Please submit a full bug report,
 with preprocessed source if appropriate.
 Please include the complete backtrace with any bug report.
 See  for instructions.
 
 Not terribly surprising given how sensitive this stuff is.  I can look 
 into why
 this fails, but looks like it can't be quite this simple, sadly.
>>> 
>>> Intersting ... a dg-torture.exp run was clean for me (all I
>>> tested...).  So yes, can you
>>> check what fails?  Maybe run the testsuite with the stage1 compiler.
>> 
>> Looks like it's the stage1 that doesn't build.  I think the difference is
>> that I was building trunk and you were building 6.  I'll try to look into
>> it later today after I get through some meetings.
> 
> Sorry, no, it was stage 2 where the failure occurs.

Ah, "good" news.  I believe the bootstrap failure with this change is another
rediscovery of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81503, which
explains why it wasn't seen for GCC 6.  I'll work on getting that fixed and
then try this again.

Bill

> 
> Bill
> 
>> 
>> Bill
>>> 
>>> Richard.
>>> 
 Bill
 
> 
> -- Bill
> 
>> On Jul 31, 2017, at 4:15 AM, Richard Biener  
>> wrote:
>> 
>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>>  wrote:
>>> Hi,
>>> 
>>> PR81354 identifies a latent bug that can happen in SLSR since the
>>> conditional candidate support was first added.  SLSR relies on the
>>> address of a GIMPLE PHI remaining constant during the course of the
>>> optimization pass, but it needs to split edges.  The use of
>>> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
>>> causes GIMPLE PHI statements to be temporarily expanded to add a
>>> predecessor, and then rebuilt to have the original number of
>>> predecessors.  The expansion usually, if not always, causes the PHI
>>> statement to change address.  Thus gimple_split_edge needs to be
>>> rewritten to perform in-situ replacement of PHI arguments.
>>> 
>>> The required pieces of make_single_succ_edge have been extracted into
>>> two places:  make_replacement_pred_edge, and some fixup code at the
>>> end of gimple_split_edge.  The division is necessary because the
>>> destination of the original edge must remember its original
>>> predecessors for the switch processing in
>>> gimple_redirect_edge_and_branch_1 to work properly.
>>> 
>>> The function gimple_redirect_edge_and_branch was factored into two
>>> pieces so that most of it can be used by gimple_split_edge without
>>> calling ssa_redirect_edge, which also interferes with PHIs.  The
>>> useful bits of ssa_redirect_edge are factored out into the next three
>>> lines of gimple_split_edge.
>>> 
>>> Similarly, redirect_eh_edge had already been similarly factored into
>>> redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
>>> and exposed redirect_eh_edge_1 for use in 
>>> gimple_redirect_edge_and_branch_1.
>>> 
>>> I've added the test from PR81354 as a torture test, 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Richard Biener]

On Tue, Aug 1, 2017 at 3:50 PM, Bill Schmidt
 wrote:
> On Aug 1, 2017, at 7:44 AM, Bill Schmidt  wrote:
>>
>>>
>>> On Aug 1, 2017, at 3:46 AM, Richard Biener  
>>> wrote:
>>>
>>> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
>>>  wrote:

> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
> wrote:
>
> That would certainly be much simpler!  I'll regstrap it and test it on 
> the other
> occurrence I've found to be certain.

 Unfortunately, this fails bootstrap:

 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
 emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
 va_list)':
 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: definition 
 in block 214 does not dominate use in block 14
 emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
 ^
 for SSA_NAME: slsr_772 in statement:
 slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
 PHI argument
 slsr_772
 for PHI node
 slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
 during GIMPLE pass: slsr
 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal compiler 
 error: verify_ssa failed
 0x11567cf3 verify_ssa(bool, bool)
   /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
 0x10fc3fff execute_function_todo
   /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
 0x10fc277f do_per_function
   /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
 0x10fc42a3 execute_todo
   /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
 Please submit a full bug report,
 with preprocessed source if appropriate.
 Please include the complete backtrace with any bug report.
 See  for instructions.

 Not terribly surprising given how sensitive this stuff is.  I can look 
 into why
 this fails, but looks like it can't be quite this simple, sadly.
>>>
>>> Intersting ... a dg-torture.exp run was clean for me (all I
>>> tested...).  So yes, can you
>>> check what fails?  Maybe run the testsuite with the stage1 compiler.
>>
>> Looks like it's the stage1 that doesn't build.  I think the difference is
>> that I was building trunk and you were building 6.  I'll try to look into
>> it later today after I get through some meetings.
>
> Sorry, no, it was stage 2 where the failure occurs.

Btw you can likely also avoid the issue in SLSR by inserting on the edge
and doing commit_edge_insertions () at the end of the pass.

Richard.

> Bill
>
>>
>> Bill
>>>
>>> Richard.
>>>
 Bill

>
> -- Bill
>
>> On Jul 31, 2017, at 4:15 AM, Richard Biener  
>> wrote:
>>
>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>>  wrote:
>>> Hi,
>>>
>>> PR81354 identifies a latent bug that can happen in SLSR since the
>>> conditional candidate support was first added.  SLSR relies on the
>>> address of a GIMPLE PHI remaining constant during the course of the
>>> optimization pass, but it needs to split edges.  The use of
>>> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
>>> causes GIMPLE PHI statements to be temporarily expanded to add a
>>> predecessor, and then rebuilt to have the original number of
>>> predecessors.  The expansion usually, if not always, causes the PHI
>>> statement to change address.  Thus gimple_split_edge needs to be
>>> rewritten to perform in-situ replacement of PHI arguments.
>>>
>>> The required pieces of make_single_succ_edge have been extracted into
>>> two places:  make_replacement_pred_edge, and some fixup code at the
>>> end of gimple_split_edge.  The division is necessary because the
>>> destination of the original edge must remember its original
>>> predecessors for the switch processing in
>>> gimple_redirect_edge_and_branch_1 to work properly.
>>>
>>> The function gimple_redirect_edge_and_branch was factored into two
>>> pieces so that most of it can be used by gimple_split_edge without
>>> calling ssa_redirect_edge, which also interferes with PHIs.  The
>>> useful bits of ssa_redirect_edge are factored out into the next three
>>> lines of gimple_split_edge.
>>>
>>> Similarly, redirect_eh_edge had already been similarly factored into
>>> redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
>>> and exposed redirect_eh_edge_1 for use in 
>>> gimple_redirect_edge_and_branch_1.
>>>
>>> I've added the test from PR81354 as a torture test, but as we've seen,
>>> small changes elsewhere in the optimizer can easily hide the problem.
>>>
>>> Bootstrapped and 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

On Aug 1, 2017, at 7:44 AM, Bill Schmidt  wrote:
> 
>> 
>> On Aug 1, 2017, at 3:46 AM, Richard Biener  
>> wrote:
>> 
>> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
>>  wrote:
>>> 
 On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
 wrote:
 
 That would certainly be much simpler!  I'll regstrap it and test it on the 
 other
 occurrence I've found to be certain.
>>> 
>>> Unfortunately, this fails bootstrap:
>>> 
>>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
>>> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
>>> va_list)':
>>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: definition 
>>> in block 214 does not dominate use in block 14
>>> emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
>>> ^
>>> for SSA_NAME: slsr_772 in statement:
>>> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
>>> PHI argument
>>> slsr_772
>>> for PHI node
>>> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
>>> during GIMPLE pass: slsr
>>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal compiler 
>>> error: verify_ssa failed
>>> 0x11567cf3 verify_ssa(bool, bool)
>>>   /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
>>> 0x10fc3fff execute_function_todo
>>>   /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
>>> 0x10fc277f do_per_function
>>>   /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
>>> 0x10fc42a3 execute_todo
>>>   /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
>>> Please submit a full bug report,
>>> with preprocessed source if appropriate.
>>> Please include the complete backtrace with any bug report.
>>> See  for instructions.
>>> 
>>> Not terribly surprising given how sensitive this stuff is.  I can look into 
>>> why
>>> this fails, but looks like it can't be quite this simple, sadly.
>> 
>> Intersting ... a dg-torture.exp run was clean for me (all I
>> tested...).  So yes, can you
>> check what fails?  Maybe run the testsuite with the stage1 compiler.
> 
> Looks like it's the stage1 that doesn't build.  I think the difference is
> that I was building trunk and you were building 6.  I'll try to look into
> it later today after I get through some meetings.

Sorry, no, it was stage 2 where the failure occurs.

Bill

> 
> Bill
>> 
>> Richard.
>> 
>>> Bill
>>> 
 
 -- Bill
 
> On Jul 31, 2017, at 4:15 AM, Richard Biener  
> wrote:
> 
> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>  wrote:
>> Hi,
>> 
>> PR81354 identifies a latent bug that can happen in SLSR since the
>> conditional candidate support was first added.  SLSR relies on the
>> address of a GIMPLE PHI remaining constant during the course of the
>> optimization pass, but it needs to split edges.  The use of
>> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
>> causes GIMPLE PHI statements to be temporarily expanded to add a
>> predecessor, and then rebuilt to have the original number of
>> predecessors.  The expansion usually, if not always, causes the PHI
>> statement to change address.  Thus gimple_split_edge needs to be
>> rewritten to perform in-situ replacement of PHI arguments.
>> 
>> The required pieces of make_single_succ_edge have been extracted into
>> two places:  make_replacement_pred_edge, and some fixup code at the
>> end of gimple_split_edge.  The division is necessary because the
>> destination of the original edge must remember its original
>> predecessors for the switch processing in
>> gimple_redirect_edge_and_branch_1 to work properly.
>> 
>> The function gimple_redirect_edge_and_branch was factored into two
>> pieces so that most of it can be used by gimple_split_edge without
>> calling ssa_redirect_edge, which also interferes with PHIs.  The
>> useful bits of ssa_redirect_edge are factored out into the next three
>> lines of gimple_split_edge.
>> 
>> Similarly, redirect_eh_edge had already been similarly factored into
>> redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
>> and exposed redirect_eh_edge_1 for use in 
>> gimple_redirect_edge_and_branch_1.
>> 
>> I've added the test from PR81354 as a torture test, but as we've seen,
>> small changes elsewhere in the optimizer can easily hide the problem.
>> 
>> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
>> Is this ok for trunk?  Eventually this needs to be backported to GCC 5,
>> 6, and 7 if that's acceptable, since PR81354 was observed on
>> gcc-5-branch.  I haven't yet prepared the backports.
> 
> I don't like make_replacement_pred_edge too 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

> On Aug 1, 2017, at 3:46 AM, Richard Biener  wrote:
> 
> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
>  wrote:
>> 
>>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
>>> wrote:
>>> 
>>> That would certainly be much simpler!  I'll regstrap it and test it on the 
>>> other
>>> occurrence I've found to be certain.
>> 
>> Unfortunately, this fails bootstrap:
>> 
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
>> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
>> va_list)':
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: definition 
>> in block 214 does not dominate use in block 14
>> emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
>> ^
>> for SSA_NAME: slsr_772 in statement:
>> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
>> PHI argument
>> slsr_772
>> for PHI node
>> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
>> during GIMPLE pass: slsr
>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal compiler 
>> error: verify_ssa failed
>> 0x11567cf3 verify_ssa(bool, bool)
>>/home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
>> 0x10fc3fff execute_function_todo
>>/home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
>> 0x10fc277f do_per_function
>>/home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
>> 0x10fc42a3 execute_todo
>>/home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
>> Please submit a full bug report,
>> with preprocessed source if appropriate.
>> Please include the complete backtrace with any bug report.
>> See  for instructions.
>> 
>> Not terribly surprising given how sensitive this stuff is.  I can look into 
>> why
>> this fails, but looks like it can't be quite this simple, sadly.
> 
> Intersting ... a dg-torture.exp run was clean for me (all I
> tested...).  So yes, can you
> check what fails?  Maybe run the testsuite with the stage1 compiler.

Looks like it's the stage1 that doesn't build.  I think the difference is
that I was building trunk and you were building 6.  I'll try to look into
it later today after I get through some meetings.

Bill
> 
> Richard.
> 
>> Bill
>> 
>>> 
>>> -- Bill
>>> 
 On Jul 31, 2017, at 4:15 AM, Richard Biener  
 wrote:
 
 On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
  wrote:
> Hi,
> 
> PR81354 identifies a latent bug that can happen in SLSR since the
> conditional candidate support was first added.  SLSR relies on the
> address of a GIMPLE PHI remaining constant during the course of the
> optimization pass, but it needs to split edges.  The use of
> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
> causes GIMPLE PHI statements to be temporarily expanded to add a
> predecessor, and then rebuilt to have the original number of
> predecessors.  The expansion usually, if not always, causes the PHI
> statement to change address.  Thus gimple_split_edge needs to be
> rewritten to perform in-situ replacement of PHI arguments.
> 
> The required pieces of make_single_succ_edge have been extracted into
> two places:  make_replacement_pred_edge, and some fixup code at the
> end of gimple_split_edge.  The division is necessary because the
> destination of the original edge must remember its original
> predecessors for the switch processing in
> gimple_redirect_edge_and_branch_1 to work properly.
> 
> The function gimple_redirect_edge_and_branch was factored into two
> pieces so that most of it can be used by gimple_split_edge without
> calling ssa_redirect_edge, which also interferes with PHIs.  The
> useful bits of ssa_redirect_edge are factored out into the next three
> lines of gimple_split_edge.
> 
> Similarly, redirect_eh_edge had already been similarly factored into
> redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
> and exposed redirect_eh_edge_1 for use in 
> gimple_redirect_edge_and_branch_1.
> 
> I've added the test from PR81354 as a torture test, but as we've seen,
> small changes elsewhere in the optimizer can easily hide the problem.
> 
> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
> Is this ok for trunk?  Eventually this needs to be backported to GCC 5,
> 6, and 7 if that's acceptable, since PR81354 was observed on
> gcc-5-branch.  I haven't yet prepared the backports.
 
 I don't like make_replacement_pred_edge too much.  Wouldn't it work
 to make sure we first shrink and then re-grow like if we simply do the
 redirect_edge_and_branch before the make_single_succ_edge call?
 At least quick testing shows it fixes the testcase on the GCC 6 branch for 
 me.
 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Richard Biener]

On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
 wrote:
>
>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt  
>> wrote:
>>
>> That would certainly be much simpler!  I'll regstrap it and test it on the 
>> other
>> occurrence I've found to be certain.
>
> Unfortunately, this fails bootstrap:
>
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* 
> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, 
> va_list)':
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: definition in 
> block 214 does not dominate use in block 14
>  emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
>  ^
> for SSA_NAME: slsr_772 in statement:
> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
> PHI argument
> slsr_772
> for PHI node
> slsr_749 = PHI <_17(26), slsr_772(14), slsr_334(214)>
> during GIMPLE pass: slsr
> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal compiler 
> error: verify_ssa failed
> 0x11567cf3 verify_ssa(bool, bool)
> /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186
> 0x10fc3fff execute_function_todo
> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997
> 0x10fc277f do_per_function
> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655
> 0x10fc42a3 execute_todo
> /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See  for instructions.
>
> Not terribly surprising given how sensitive this stuff is.  I can look into 
> why
> this fails, but looks like it can't be quite this simple, sadly.

Intersting ... a dg-torture.exp run was clean for me (all I
tested...).  So yes, can you
check what fails?  Maybe run the testsuite with the stage1 compiler.

Richard.

> Bill
>
>>
>> -- Bill
>>
>>> On Jul 31, 2017, at 4:15 AM, Richard Biener  
>>> wrote:
>>>
>>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>>>  wrote:
 Hi,

 PR81354 identifies a latent bug that can happen in SLSR since the
 conditional candidate support was first added.  SLSR relies on the
 address of a GIMPLE PHI remaining constant during the course of the
 optimization pass, but it needs to split edges.  The use of
 make_single_succ_edge and reinstall_phi_args in gimple_split_edge
 causes GIMPLE PHI statements to be temporarily expanded to add a
 predecessor, and then rebuilt to have the original number of
 predecessors.  The expansion usually, if not always, causes the PHI
 statement to change address.  Thus gimple_split_edge needs to be
 rewritten to perform in-situ replacement of PHI arguments.

 The required pieces of make_single_succ_edge have been extracted into
 two places:  make_replacement_pred_edge, and some fixup code at the
 end of gimple_split_edge.  The division is necessary because the
 destination of the original edge must remember its original
 predecessors for the switch processing in
 gimple_redirect_edge_and_branch_1 to work properly.

 The function gimple_redirect_edge_and_branch was factored into two
 pieces so that most of it can be used by gimple_split_edge without
 calling ssa_redirect_edge, which also interferes with PHIs.  The
 useful bits of ssa_redirect_edge are factored out into the next three
 lines of gimple_split_edge.

 Similarly, redirect_eh_edge had already been similarly factored into
 redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
 and exposed redirect_eh_edge_1 for use in 
 gimple_redirect_edge_and_branch_1.

 I've added the test from PR81354 as a torture test, but as we've seen,
 small changes elsewhere in the optimizer can easily hide the problem.

 Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
 Is this ok for trunk?  Eventually this needs to be backported to GCC 5,
 6, and 7 if that's acceptable, since PR81354 was observed on
 gcc-5-branch.  I haven't yet prepared the backports.
>>>
>>> I don't like make_replacement_pred_edge too much.  Wouldn't it work
>>> to make sure we first shrink and then re-grow like if we simply do the
>>> redirect_edge_and_branch before the make_single_succ_edge call?
>>> At least quick testing shows it fixes the testcase on the GCC 6 branch for 
>>> me.
>>>
>>> Index: gcc/tree-cfg.c
>>> ===
>>> --- gcc/tree-cfg.c  (revision 250732)
>>> +++ gcc/tree-cfg.c  (working copy)
>>> @@ -2753,12 +2753,16 @@ gimple_split_edge (edge edge_in)
>>>  new_bb = create_empty_bb (after_bb);
>>>  new_bb->frequency = EDGE_FREQUENCY (edge_in);
>>>  new_bb->count = edge_in->count;
>>> +
>>> +  /* First redirect the 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

That would certainly be much simpler!  I'll regstrap it and test it on the other
occurrence I've found to be certain.

-- Bill

> On Jul 31, 2017, at 4:15 AM, Richard Biener  
> wrote:
> 
> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>  wrote:
>> Hi,
>> 
>> PR81354 identifies a latent bug that can happen in SLSR since the
>> conditional candidate support was first added.  SLSR relies on the
>> address of a GIMPLE PHI remaining constant during the course of the
>> optimization pass, but it needs to split edges.  The use of
>> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
>> causes GIMPLE PHI statements to be temporarily expanded to add a
>> predecessor, and then rebuilt to have the original number of
>> predecessors.  The expansion usually, if not always, causes the PHI
>> statement to change address.  Thus gimple_split_edge needs to be
>> rewritten to perform in-situ replacement of PHI arguments.
>> 
>> The required pieces of make_single_succ_edge have been extracted into
>> two places:  make_replacement_pred_edge, and some fixup code at the
>> end of gimple_split_edge.  The division is necessary because the
>> destination of the original edge must remember its original
>> predecessors for the switch processing in
>> gimple_redirect_edge_and_branch_1 to work properly.
>> 
>> The function gimple_redirect_edge_and_branch was factored into two
>> pieces so that most of it can be used by gimple_split_edge without
>> calling ssa_redirect_edge, which also interferes with PHIs.  The
>> useful bits of ssa_redirect_edge are factored out into the next three
>> lines of gimple_split_edge.
>> 
>> Similarly, redirect_eh_edge had already been similarly factored into
>> redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
>> and exposed redirect_eh_edge_1 for use in gimple_redirect_edge_and_branch_1.
>> 
>> I've added the test from PR81354 as a torture test, but as we've seen,
>> small changes elsewhere in the optimizer can easily hide the problem.
>> 
>> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
>> Is this ok for trunk?  Eventually this needs to be backported to GCC 5,
>> 6, and 7 if that's acceptable, since PR81354 was observed on
>> gcc-5-branch.  I haven't yet prepared the backports.
> 
> I don't like make_replacement_pred_edge too much.  Wouldn't it work
> to make sure we first shrink and then re-grow like if we simply do the
> redirect_edge_and_branch before the make_single_succ_edge call?
> At least quick testing shows it fixes the testcase on the GCC 6 branch for me.
> 
> Index: gcc/tree-cfg.c
> ===
> --- gcc/tree-cfg.c  (revision 250732)
> +++ gcc/tree-cfg.c  (working copy)
> @@ -2753,12 +2753,16 @@ gimple_split_edge (edge edge_in)
>   new_bb = create_empty_bb (after_bb);
>   new_bb->frequency = EDGE_FREQUENCY (edge_in);
>   new_bb->count = edge_in->count;
> +
> +  /* First redirect the existing edge to avoid reallocating
> + PHI nodes in dest.  */
> +  e = redirect_edge_and_branch (edge_in, new_bb);
> +  gcc_assert (e == edge_in);
> +
>   new_edge = make_edge (new_bb, dest, EDGE_FALLTHRU);
>   new_edge->probability = REG_BR_PROB_BASE;
>   new_edge->count = edge_in->count;
> 
> -  e = redirect_edge_and_branch (edge_in, new_bb);
> -  gcc_assert (e == edge_in);
>   reinstall_phi_args (new_edge, e);
> 
>   return new_bb;
> 
> Sorry for misleading you to a complex solution.
> 
> Thanks,
> Richard.
> 
>> Thanks,
>> Bill
>> 
>> 
>> [gcc]
>> 
>> 2017-07-30  Bill Schmidt  
>> 
>>PR tree-optimization/81354
>>* tree-cfg.c (gimple_redirect_edge_and_branch_1): New decl.
>>(reinstall_phi_args): Delete function.
>>(make_replacement_pred_edge): New function.
>>(gimple_split_edge): Rewrite.
>>(gimple_redirect_edge_and_branch_1): New function, factored
>>from...
>>(gimple_redirect_edge_and_branch): ...here.
>>(split_critical_edges): Don't re-split already split edges.
>>* tree-eh.c (redirect_eh_edge_1): Make visible.
>>* tree-eh.h (redirect_eh_edge_1): Likewise.
>> 
>> [gcc/testsuite]
>> 
>> 2017-07-30  Bill Schmidt  
>> 
>>PR tree-optimization/81354
>>* g++.dg/torture/pr81354.C: New file.
>> 
>> 
>> Index: gcc/testsuite/g++.dg/torture/pr81354.C
>> ===
>> --- gcc/testsuite/g++.dg/torture/pr81354.C  (nonexistent)
>> +++ gcc/testsuite/g++.dg/torture/pr81354.C  (working copy)
>> @@ -0,0 +1,24 @@
>> +// PR81354 reported this test as crashing in a limited range of revisions.
>> +// { dg-do compile }
>> +
>> +struct T { double a; double b; };
>> +
>> +void foo(T Ad[], int As[2])
>> +{
>> +  int j;
>> +  int i;
>> +  int Bs[2] = {0,0};
>> +  T Bd[16];
>> +
>> +  for (j = 0; j < 4; j++) {
>> +for (i = 0; i + 

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Richard Biener]

On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
 wrote:
> Hi,
>
> PR81354 identifies a latent bug that can happen in SLSR since the
> conditional candidate support was first added.  SLSR relies on the
> address of a GIMPLE PHI remaining constant during the course of the
> optimization pass, but it needs to split edges.  The use of
> make_single_succ_edge and reinstall_phi_args in gimple_split_edge
> causes GIMPLE PHI statements to be temporarily expanded to add a
> predecessor, and then rebuilt to have the original number of
> predecessors.  The expansion usually, if not always, causes the PHI
> statement to change address.  Thus gimple_split_edge needs to be
> rewritten to perform in-situ replacement of PHI arguments.
>
> The required pieces of make_single_succ_edge have been extracted into
> two places:  make_replacement_pred_edge, and some fixup code at the
> end of gimple_split_edge.  The division is necessary because the
> destination of the original edge must remember its original
> predecessors for the switch processing in
> gimple_redirect_edge_and_branch_1 to work properly.
>
> The function gimple_redirect_edge_and_branch was factored into two
> pieces so that most of it can be used by gimple_split_edge without
> calling ssa_redirect_edge, which also interferes with PHIs.  The
> useful bits of ssa_redirect_edge are factored out into the next three
> lines of gimple_split_edge.
>
> Similarly, redirect_eh_edge had already been similarly factored into
> redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
> and exposed redirect_eh_edge_1 for use in gimple_redirect_edge_and_branch_1.
>
> I've added the test from PR81354 as a torture test, but as we've seen,
> small changes elsewhere in the optimizer can easily hide the problem.
>
> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
> Is this ok for trunk?  Eventually this needs to be backported to GCC 5,
> 6, and 7 if that's acceptable, since PR81354 was observed on
> gcc-5-branch.  I haven't yet prepared the backports.

I don't like make_replacement_pred_edge too much.  Wouldn't it work
to make sure we first shrink and then re-grow like if we simply do the
redirect_edge_and_branch before the make_single_succ_edge call?
At least quick testing shows it fixes the testcase on the GCC 6 branch for me.

Index: gcc/tree-cfg.c
===
--- gcc/tree-cfg.c  (revision 250732)
+++ gcc/tree-cfg.c  (working copy)
@@ -2753,12 +2753,16 @@ gimple_split_edge (edge edge_in)
   new_bb = create_empty_bb (after_bb);
   new_bb->frequency = EDGE_FREQUENCY (edge_in);
   new_bb->count = edge_in->count;
+
+  /* First redirect the existing edge to avoid reallocating
+ PHI nodes in dest.  */
+  e = redirect_edge_and_branch (edge_in, new_bb);
+  gcc_assert (e == edge_in);
+
   new_edge = make_edge (new_bb, dest, EDGE_FALLTHRU);
   new_edge->probability = REG_BR_PROB_BASE;
   new_edge->count = edge_in->count;

-  e = redirect_edge_and_branch (edge_in, new_bb);
-  gcc_assert (e == edge_in);
   reinstall_phi_args (new_edge, e);

   return new_bb;

Sorry for misleading you to a complex solution.

Thanks,
Richard.

> Thanks,
> Bill
>
>
> [gcc]
>
> 2017-07-30  Bill Schmidt  
>
> PR tree-optimization/81354
> * tree-cfg.c (gimple_redirect_edge_and_branch_1): New decl.
> (reinstall_phi_args): Delete function.
> (make_replacement_pred_edge): New function.
> (gimple_split_edge): Rewrite.
> (gimple_redirect_edge_and_branch_1): New function, factored
> from...
> (gimple_redirect_edge_and_branch): ...here.
> (split_critical_edges): Don't re-split already split edges.
> * tree-eh.c (redirect_eh_edge_1): Make visible.
> * tree-eh.h (redirect_eh_edge_1): Likewise.
>
> [gcc/testsuite]
>
> 2017-07-30  Bill Schmidt  
>
> PR tree-optimization/81354
> * g++.dg/torture/pr81354.C: New file.
>
>
> Index: gcc/testsuite/g++.dg/torture/pr81354.C
> ===
> --- gcc/testsuite/g++.dg/torture/pr81354.C  (nonexistent)
> +++ gcc/testsuite/g++.dg/torture/pr81354.C  (working copy)
> @@ -0,0 +1,24 @@
> +// PR81354 reported this test as crashing in a limited range of revisions.
> +// { dg-do compile }
> +
> +struct T { double a; double b; };
> +
> +void foo(T Ad[], int As[2])
> +{
> +  int j;
> +  int i;
> +  int Bs[2] = {0,0};
> +  T Bd[16];
> +
> +  for (j = 0; j < 4; j++) {
> +for (i = 0; i + 1 <= j + 1; i++) {
> +  Ad[i + As[0] * j] = Bd[i + Bs[0] * j];
> +}
> +
> +i = j + 1;  // <- comment out this line and it does not crash
> +for (; i + 1 < 5; i++) {
> +  Ad[i + As[0] * j].a = 0.0;
> +  Ad[i + As[0] * j].b = 0.0;
> +}
> +  }
> +}
> Index: gcc/tree-cfg.c
> ===

[PATCH] Fix PR81354 (rewrite gimple_split_edge)

[Bill Schmidt]

Hi,

PR81354 identifies a latent bug that can happen in SLSR since the
conditional candidate support was first added.  SLSR relies on the
address of a GIMPLE PHI remaining constant during the course of the
optimization pass, but it needs to split edges.  The use of
make_single_succ_edge and reinstall_phi_args in gimple_split_edge
causes GIMPLE PHI statements to be temporarily expanded to add a
predecessor, and then rebuilt to have the original number of
predecessors.  The expansion usually, if not always, causes the PHI
statement to change address.  Thus gimple_split_edge needs to be
rewritten to perform in-situ replacement of PHI arguments.

The required pieces of make_single_succ_edge have been extracted into
two places:  make_replacement_pred_edge, and some fixup code at the
end of gimple_split_edge.  The division is necessary because the
destination of the original edge must remember its original
predecessors for the switch processing in
gimple_redirect_edge_and_branch_1 to work properly.

The function gimple_redirect_edge_and_branch was factored into two
pieces so that most of it can be used by gimple_split_edge without
calling ssa_redirect_edge, which also interferes with PHIs.  The
useful bits of ssa_redirect_edge are factored out into the next three
lines of gimple_split_edge.

Similarly, redirect_eh_edge had already been similarly factored into
redirect_eh_edge_1 and ssa_redirect_edge.  I took advantage of that
and exposed redirect_eh_edge_1 for use in gimple_redirect_edge_and_branch_1.

I've added the test from PR81354 as a torture test, but as we've seen,
small changes elsewhere in the optimizer can easily hide the problem.

Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
Is this ok for trunk?  Eventually this needs to be backported to GCC 5,
6, and 7 if that's acceptable, since PR81354 was observed on
gcc-5-branch.  I haven't yet prepared the backports.

Thanks,
Bill


[gcc]

2017-07-30  Bill Schmidt  

PR tree-optimization/81354
* tree-cfg.c (gimple_redirect_edge_and_branch_1): New decl.
(reinstall_phi_args): Delete function.
(make_replacement_pred_edge): New function.
(gimple_split_edge): Rewrite.
(gimple_redirect_edge_and_branch_1): New function, factored
from...
(gimple_redirect_edge_and_branch): ...here.
(split_critical_edges): Don't re-split already split edges.
* tree-eh.c (redirect_eh_edge_1): Make visible.
* tree-eh.h (redirect_eh_edge_1): Likewise.

[gcc/testsuite]

2017-07-30  Bill Schmidt  

PR tree-optimization/81354
* g++.dg/torture/pr81354.C: New file.


Index: gcc/testsuite/g++.dg/torture/pr81354.C
===
--- gcc/testsuite/g++.dg/torture/pr81354.C  (nonexistent)
+++ gcc/testsuite/g++.dg/torture/pr81354.C  (working copy)
@@ -0,0 +1,24 @@
+// PR81354 reported this test as crashing in a limited range of revisions.
+// { dg-do compile }
+
+struct T { double a; double b; };
+
+void foo(T Ad[], int As[2])
+{
+  int j;
+  int i;
+  int Bs[2] = {0,0};
+  T Bd[16];
+
+  for (j = 0; j < 4; j++) {
+for (i = 0; i + 1 <= j + 1; i++) {
+  Ad[i + As[0] * j] = Bd[i + Bs[0] * j];
+}
+
+i = j + 1;  // <- comment out this line and it does not crash
+for (; i + 1 < 5; i++) {
+  Ad[i + As[0] * j].a = 0.0;
+  Ad[i + As[0] * j].b = 0.0;
+}
+  }
+}
Index: gcc/tree-cfg.c
===
--- gcc/tree-cfg.c  (revision 250721)
+++ gcc/tree-cfg.c  (working copy)
@@ -154,6 +154,7 @@ static void make_gimple_switch_edges (gswitch *, b
 static bool make_goto_expr_edges (basic_block);
 static void make_gimple_asm_edges (basic_block);
 static edge gimple_redirect_edge_and_branch (edge, basic_block);
+static edge gimple_redirect_edge_and_branch_1 (edge, basic_block);
 static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
 
 /* Various helpers.  */
@@ -2776,35 +2777,6 @@ last_and_only_stmt (basic_block bb)
 return NULL;
 }
 
-/* Reinstall those PHI arguments queued in OLD_EDGE to NEW_EDGE.  */
-
-static void
-reinstall_phi_args (edge new_edge, edge old_edge)
-{
-  edge_var_map *vm;
-  int i;
-  gphi_iterator phis;
-
-  vec *v = redirect_edge_var_map_vector (old_edge);
-  if (!v)
-return;
-
-  for (i = 0, phis = gsi_start_phis (new_edge->dest);
-   v->iterate (i, ) && !gsi_end_p (phis);
-   i++, gsi_next ())
-{
-  gphi *phi = phis.phi ();
-  tree result = redirect_edge_var_map_result (vm);
-  tree arg = redirect_edge_var_map_def (vm);
-
-  gcc_assert (result == gimple_phi_result (phi));
-
-  add_phi_arg (phi, arg, new_edge, redirect_edge_var_map_location (vm));
-}
-
-  redirect_edge_var_map_clear (old_edge);
-}
-
 /* Returns the basic block after which the new basic block created
by splitting edge EDGE_IN should