diff mbox series

cgraph: remove dead if stmt in build_cgraph_edges pass

Message ID 20241024143724.3790409-1-melcrjos@fit.cvut.cz
State New
Headers
Series cgraph: remove dead if stmt in build_cgraph_edges pass |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Josef Melcr Oct. 24, 2024, 2:37 p.m. UTC 
  This patch removes a dead if statement checking for gomp-parallel gimple
statements. This if is in the execute method of build_cgraph_edges pass,
which is executed right after the omp_expand pass, which removes these
gimple statements and replaces them with simple gcalls, making this if
practically dead. 

Some TSan tests are failing with this patch, but I don't think this
change is likely to cause these failures. Additionally, the failures are
not consistent across runs, making me think these failures are a
bug in TSan itself. All other tests are ok. Tested on x86_64-pc-linux-gnu. 

OK for master ?

gcc/ChangeLog:

	* cgraphbuild.cc (pass_build_cgraph_edges::execute): remove if
	  statement checking for gomp-parallel statements

Signed-off-by: Josef Melcr <melcrjos@fit.cvut.cz>
---
 gcc/cgraphbuild.cc | 6 ------
 1 file changed, 6 deletions(-)

Comments

Jakub Jelinek Oct. 24, 2024, 4:49 p.m. UTC | #1 
On Thu, Oct 24, 2024 at 04:37:24PM +0200, Josef Melcr wrote:
> This patch removes a dead if statement checking for gomp-parallel gimple
> statements. This if is in the execute method of build_cgraph_edges pass,
> which is executed right after the omp_expand pass, which removes these
> gimple statements and replaces them with simple gcalls, making this if
> practically dead. 
> 
> Some TSan tests are failing with this patch, but I don't think this
> change is likely to cause these failures. Additionally, the failures are
> not consistent across runs, making me think these failures are a
> bug in TSan itself. All other tests are ok. Tested on x86_64-pc-linux-gnu. 
> 
> OK for master ?
> 
> gcc/ChangeLog:
> 
> 	* cgraphbuild.cc (pass_build_cgraph_edges::execute): remove if

Capital Remove

> 	  statement checking for gomp-parallel statements

The second line should be just tab indented, not tab + 2 spaces, and
finished with dot.  gomp_parallel rather than gomp-parallel.

> --- a/gcc/cgraphbuild.cc
> +++ b/gcc/cgraphbuild.cc
> @@ -340,12 +340,6 @@ pass_build_cgraph_edges::execute (function *fun)
>  					    bb->count);
>  	    }
>  	  node->record_stmt_references (stmt);
> -	  if (gomp_parallel *omp_par_stmt = dyn_cast <gomp_parallel *> (stmt))
> -	    {
> -	      tree fn = gimple_omp_parallel_child_fn (omp_par_stmt);
> -	      node->create_reference (cgraph_node::get_create (fn),
> -				      IPA_REF_ADDR, stmt);
> -	    }
>  	  if (gimple_code (stmt) == GIMPLE_OMP_TASK)
>  	    {
>  	      tree fn = gimple_omp_task_child_fn (stmt);

The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be useful
as replacement at least for some time to verify it isn't needed (but if it
would be needed, we miss various other GIMPLE_OMP_* statements).

	Jakub
Josef Melcr Oct. 24, 2024, 5:45 p.m. UTC | #2 
> Capital Remove
> The second line should be just tab indented, not tab + 2 spaces, and
> finished with dot.  gomp_parallel rather than gomp-parallel.
Sorry about the formatting issues, I didn't notice them.

> The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
> Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be useful
> as replacement at least for some time to verify it isn't needed (but if it
> would be needed, we miss various other GIMPLE_OMP_* statements).
The GIMPLE_OMP_TASK case went under my radar, since I was primarily 
focused on gomp_parallel statements.  I'll try these changes and retest.


Best regards,

Josef Melcr

Dne 24. 10. 24 v 18:49 Jakub Jelinek napsal(a):
> On Thu, Oct 24, 2024 at 04:37:24PM +0200, Josef Melcr wrote:
>> This patch removes a dead if statement checking for gomp-parallel gimple
>> statements. This if is in the execute method of build_cgraph_edges pass,
>> which is executed right after the omp_expand pass, which removes these
>> gimple statements and replaces them with simple gcalls, making this if
>> practically dead.
>>
>> Some TSan tests are failing with this patch, but I don't think this
>> change is likely to cause these failures. Additionally, the failures are
>> not consistent across runs, making me think these failures are a
>> bug in TSan itself. All other tests are ok. Tested on x86_64-pc-linux-gnu.
>>
>> OK for master ?
>>
>> gcc/ChangeLog:
>>
>> 	* cgraphbuild.cc (pass_build_cgraph_edges::execute): remove if
> Capital Remove
>
>> 	  statement checking for gomp-parallel statements
> The second line should be just tab indented, not tab + 2 spaces, and
> finished with dot.  gomp_parallel rather than gomp-parallel.
>
>> --- a/gcc/cgraphbuild.cc
>> +++ b/gcc/cgraphbuild.cc
>> @@ -340,12 +340,6 @@ pass_build_cgraph_edges::execute (function *fun)
>>   					    bb->count);
>>   	    }
>>   	  node->record_stmt_references (stmt);
>> -	  if (gomp_parallel *omp_par_stmt = dyn_cast <gomp_parallel *> (stmt))
>> -	    {
>> -	      tree fn = gimple_omp_parallel_child_fn (omp_par_stmt);
>> -	      node->create_reference (cgraph_node::get_create (fn),
>> -				      IPA_REF_ADDR, stmt);
>> -	    }
>>   	  if (gimple_code (stmt) == GIMPLE_OMP_TASK)
>>   	    {
>>   	      tree fn = gimple_omp_task_child_fn (stmt);
> The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
> Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be useful
> as replacement at least for some time to verify it isn't needed (but if it
> would be needed, we miss various other GIMPLE_OMP_* statements).
>
> 	Jakub
>
Josef Melcr Oct. 25, 2024, 5:19 a.m. UTC | #3 
So I experimented a little and ran the testsuite a few times. While both 
if statements seem to be dead, the assertion gcc_checking_assert 
(!is_gimple_omp (stmt)) doesn't actually hold, as adding this assert 
breaks around 40 omp/oacc tests, so some other statements are definitely 
slipping through. I would need to dig deeper to see which statements 
those are. I am not quite sure where that leaves this patch.


Best regards,

Josef Melcr

Dne 24. 10. 24 v 19:45 Josef Melcr napsal(a):

>> Capital Remove
>> The second line should be just tab indented, not tab + 2 spaces, and
>> finished with dot.  gomp_parallel rather than gomp-parallel.
> Sorry about the formatting issues, I didn't notice them.
>
>> The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
>> Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be 
>> useful
>> as replacement at least for some time to verify it isn't needed (but 
>> if it
>> would be needed, we miss various other GIMPLE_OMP_* statements).
> The GIMPLE_OMP_TASK case went under my radar, since I was primarily 
> focused on gomp_parallel statements.  I'll try these changes and retest.
>
>
> Best regards,
>
> Josef Melcr
>
> Dne 24. 10. 24 v 18:49 Jakub Jelinek napsal(a):
>> On Thu, Oct 24, 2024 at 04:37:24PM +0200, Josef Melcr wrote:
>>> This patch removes a dead if statement checking for gomp-parallel 
>>> gimple
>>> statements. This if is in the execute method of build_cgraph_edges 
>>> pass,
>>> which is executed right after the omp_expand pass, which removes these
>>> gimple statements and replaces them with simple gcalls, making this if
>>> practically dead.
>>>
>>> Some TSan tests are failing with this patch, but I don't think this
>>> change is likely to cause these failures. Additionally, the failures 
>>> are
>>> not consistent across runs, making me think these failures are a
>>> bug in TSan itself. All other tests are ok. Tested on 
>>> x86_64-pc-linux-gnu.
>>>
>>> OK for master ?
>>>
>>> gcc/ChangeLog:
>>>
>>>     * cgraphbuild.cc (pass_build_cgraph_edges::execute): remove if
>> Capital Remove
>>
>>>       statement checking for gomp-parallel statements
>> The second line should be just tab indented, not tab + 2 spaces, and
>> finished with dot.  gomp_parallel rather than gomp-parallel.
>>
>>> --- a/gcc/cgraphbuild.cc
>>> +++ b/gcc/cgraphbuild.cc
>>> @@ -340,12 +340,6 @@ pass_build_cgraph_edges::execute (function *fun)
>>>                           bb->count);
>>>           }
>>>         node->record_stmt_references (stmt);
>>> -      if (gomp_parallel *omp_par_stmt = dyn_cast <gomp_parallel *> 
>>> (stmt))
>>> -        {
>>> -          tree fn = gimple_omp_parallel_child_fn (omp_par_stmt);
>>> -          node->create_reference (cgraph_node::get_create (fn),
>>> -                      IPA_REF_ADDR, stmt);
>>> -        }
>>>         if (gimple_code (stmt) == GIMPLE_OMP_TASK)
>>>           {
>>>             tree fn = gimple_omp_task_child_fn (stmt);
>> The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
>> Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be 
>> useful
>> as replacement at least for some time to verify it isn't needed (but 
>> if it
>> would be needed, we miss various other GIMPLE_OMP_* statements).
>>
>>     Jakub
>>
Josef Melcr Nov. 3, 2024, 10:22 p.m. UTC | #4 
Hi,

I've looked at the statements slipping through and they are all atomic 
loads and stores. What I find strange is these statements don't get 
expanded only in tests for errors, when the code is not supposed to 
compile. In all other cases all statements get expanded just fine, 
including atomic loads and stores. I am kind of at a loss for why that's 
happening, I was hoping maybe somebody here could provide some insight ? 
Thanks in advance :)


Best regards,

Josef Melcr

Dne 25. 10. 24 v 7:19 Josef Melcr napsal(a):
> So I experimented a little and ran the testsuite a few times. While 
> both if statements seem to be dead, the assertion gcc_checking_assert 
> (!is_gimple_omp (stmt)) doesn't actually hold, as adding this assert 
> breaks around 40 omp/oacc tests, so some other statements are 
> definitely slipping through. I would need to dig deeper to see which 
> statements those are. I am not quite sure where that leaves this patch.
>
>
> Best regards,
>
> Josef Melcr
>
> Dne 24. 10. 24 v 19:45 Josef Melcr napsal(a):
>
>>> Capital Remove
>>> The second line should be just tab indented, not tab + 2 spaces, and
>>> finished with dot.  gomp_parallel rather than gomp-parallel.
>> Sorry about the formatting issues, I didn't notice them.
>>
>>> The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
>>> Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be 
>>> useful
>>> as replacement at least for some time to verify it isn't needed (but 
>>> if it
>>> would be needed, we miss various other GIMPLE_OMP_* statements).
>> The GIMPLE_OMP_TASK case went under my radar, since I was primarily 
>> focused on gomp_parallel statements.  I'll try these changes and retest.
>>
>>
>> Best regards,
>>
>> Josef Melcr
>>
>> Dne 24. 10. 24 v 18:49 Jakub Jelinek napsal(a):
>>> On Thu, Oct 24, 2024 at 04:37:24PM +0200, Josef Melcr wrote:
>>>> This patch removes a dead if statement checking for gomp-parallel 
>>>> gimple
>>>> statements. This if is in the execute method of build_cgraph_edges 
>>>> pass,
>>>> which is executed right after the omp_expand pass, which removes these
>>>> gimple statements and replaces them with simple gcalls, making this if
>>>> practically dead.
>>>>
>>>> Some TSan tests are failing with this patch, but I don't think this
>>>> change is likely to cause these failures. Additionally, the 
>>>> failures are
>>>> not consistent across runs, making me think these failures are a
>>>> bug in TSan itself. All other tests are ok. Tested on 
>>>> x86_64-pc-linux-gnu.
>>>>
>>>> OK for master ?
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>     * cgraphbuild.cc (pass_build_cgraph_edges::execute): remove if
>>> Capital Remove
>>>
>>>>       statement checking for gomp-parallel statements
>>> The second line should be just tab indented, not tab + 2 spaces, and
>>> finished with dot.  gomp_parallel rather than gomp-parallel.
>>>
>>>> --- a/gcc/cgraphbuild.cc
>>>> +++ b/gcc/cgraphbuild.cc
>>>> @@ -340,12 +340,6 @@ pass_build_cgraph_edges::execute (function *fun)
>>>>                           bb->count);
>>>>           }
>>>>         node->record_stmt_references (stmt);
>>>> -      if (gomp_parallel *omp_par_stmt = dyn_cast <gomp_parallel *> 
>>>> (stmt))
>>>> -        {
>>>> -          tree fn = gimple_omp_parallel_child_fn (omp_par_stmt);
>>>> -          node->create_reference (cgraph_node::get_create (fn),
>>>> -                      IPA_REF_ADDR, stmt);
>>>> -        }
>>>>         if (gimple_code (stmt) == GIMPLE_OMP_TASK)
>>>>           {
>>>>             tree fn = gimple_omp_task_child_fn (stmt);
>>> The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.
>>> Wonder if gcc_checking_assert (!is_gimple_omp (stmt)); wouldn't be 
>>> useful
>>> as replacement at least for some time to verify it isn't needed (but 
>>> if it
>>> would be needed, we miss various other GIMPLE_OMP_* statements).
>>>
>>>     Jakub
>>>
diff mbox series

Patch

diff --git a/gcc/cgraphbuild.cc b/gcc/cgraphbuild.cc
index 8852ba9ee92..191ec1077e4 100644
--- a/gcc/cgraphbuild.cc
+++ b/gcc/cgraphbuild.cc
@@ -340,12 +340,6 @@  pass_build_cgraph_edges::execute (function *fun)
 					    bb->count);
 	    }
 	  node->record_stmt_references (stmt);
-	  if (gomp_parallel *omp_par_stmt = dyn_cast <gomp_parallel *> (stmt))
-	    {
-	      tree fn = gimple_omp_parallel_child_fn (omp_par_stmt);
-	      node->create_reference (cgraph_node::get_create (fn),
-				      IPA_REF_ADDR, stmt);
-	    }
 	  if (gimple_code (stmt) == GIMPLE_OMP_TASK)
 	    {
 	      tree fn = gimple_omp_task_child_fn (stmt);