Re: [PATCH] cgraph: remove dead if stmt in build_cgraph_edges pass

[Josef Melcr]

Hi,





Best regards,

Josef Melcr

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

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 :)

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.

would be needed, we miss various other GIMPLE_OMP_* statements).

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

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

not consistent across runs, making me think these failures are a


OK for master ?

gcc/ChangeLog:

    * cgraphbuild.cc (pass_build_cgraph_edges::execute): remove if

change is likely to cause these failures. Additionally, the failures are bug in TSan itself. All other tests are ok. Tested on x86_64-pc-linux-gnu.

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);

-        {
-          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);

-      if (gomp_parallel *omp_par_stmt = dyn_cast <gomp_parallel *> (stmt))

The if (gimple_code (stmt) == GIMPLE_OMP_TASK) case should go as well.

would be needed, we miss various other GIMPLE_OMP_* statements).

    Jakub

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

smime.p7s
Description: Elektronicky podpis S/MIME