Hi,
thank you for the review :) Below are some of my thoughts, I will
implement your suggestions as soon as I can.
On 9/20/25 00:29, Martin Jambor wrote:
One more naming nit. In various comments and dumping strings, you use
the term "offloading" function. I guess the term is somewhat
understandable in the context of OpenMP/OpenACC (but there it usually
means something offloading computation to a GPU or other such special
device, not running GOMP_parallel). I'd suggest "callback-dispatching"
or something similar and more general (qsort is not offloading in any
way).
I do use the term "offloading" very loosely and often times
incorrectly. I think "callback-dispatching" is great, so I will use
that instead. I apologize if it caused any confusion.
gcc/testsuite/ChangeLog:
* gcc.dg/ipa/ipcp-cb-spec1.c: New test.
* gcc.dg/ipa/ipcp-cb-spec2.c: New test.
* gcc.dg/ipa/ipcp-cb1.c: New test.
It seems you have lost the test in testsuite/gcc.dg/attr-callback.c ?
It looked useful.
The test serves no purpose, as the attribute is not in the public API at
the moment (suggested in
https://gcc.gnu.org/pipermail/gcc-patches/2025-June/686859.html and
implemented in v2). It will of course make a return once a consensus
about the attribute is reached, but right now we would be adding a test
that cannot be passed.
+ cgraph_edge *edge = (cgraph_edge *) *slot;
+ gcc_assert (edge->speculative || edge->has_callback);
+ if (edge->has_callback)
+ /* If the slot is already occupied, then the hashed edge is the
+ callback-carrying edge, which is desired behavior, so we can safely
+ return. */
+ return;
if (e->callee && (!e->prev_callee
|| !e->prev_callee->speculative
|| e->prev_callee->call_stmt != e->call_stmt))
I still don't understand why the early exit is not enough.
In one of your previous emails you wrote:
In some cases, the callback edge would be added before the
callback-carrying edge, occupying its slot and leading to a crash on the
assert when adding the callback-carrying edge. The early return
prevents that. Sadly, I am afraid I no longer have the code snippet to
reproduce the crash.
...and I'd think that the early return would be enough and the second
condition is not necessary. But if it is not, I don't have an issue
with the code as it is.
Right now I can't think of a case where the early return is not enough,
I'll look into it. It's been a while since I've worked on it, so I am
not entirely sure, but you are probably right.
+ /* When updating a callback or a callback-carrying edge, update every edge
+ involved. */
+ if (update_derived_edges && (e->callback || e->has_callback))
+ {
+ cgraph_edge *current, *next, *carrying;
+ carrying = e->has_callback ? e : e->get_callback_carrying_edge ();
+
+ current = e->first_callback_edge ();
+ if (current)
+ {
+ for (cgraph_edge *d = current; d; d = next)
+ {
+ next = d->next_callback_edge ();
+ cgraph_edge *d2 = set_call_stmt (d, new_stmt, false);
+ gcc_assert (d2 == d);
+ }
+ }
+ carrying = set_call_stmt (carrying, new_stmt, false);
+ return carrying;
+ }
+
I'm not sure that always returning the carrying edge is the expected
thing to do. But it does not look like it ever matters that the
original callback edge is returned, so perhaps it is OK to just document
the behavior in the function comment - ah, it actually is done in the
header file, so please just copy it to the .c file description too.
(Though perhaps Honza will have a different opinion about this.)
It doesn't really matter which edge is returned, as any of the related
edges can be obtained with the provided methods. The carrying edge is
usually more useful when calling set_call_stmt, it is the "owner" of the
call statement (the call statement is calling the function the carrying
edge is pointing to) and I also try to return the carrying edge whenever
performing some action on all of the edges at once, for the sake of
consistency. For these reasons, I chose to return the carrying edge,
though I can see it not being entirely obvious behavior. I can change
it to returning the original edge if you want me to, though I think the
carrying edge is better suited.
+ /* Used to pair callback edges and the attributes that originated them
+ together. Needed in order to get ipa-icf to work with callbacks.
I think the sentence about ICF is outdated? In any case it seems quite
unnecessary.
It's not outdated, as without the callback_id we wouldn't know how to
redirect callback edges affected by icf, though mentioning it is
probably unnecessary. I will remove it.
+/* Initializes ipa_edge_args summary of CBE given its callback-carrying edge.
+ This primarily means allocating the correct amount of jump functions. */
+
+static inline void
+init_callback_edge_summary (struct cgraph_edge *carrying,
+ struct cgraph_edge *cbe)
+{
+ ipa_edge_args *carrying_args = ipa_edge_args_sum->get (carrying);
+ ipa_edge_args *cb_args = ipa_edge_args_sum->get_create (cbe);
+ vec_safe_grow_cleared (cb_args->jump_functions,
+ carrying_args->jump_functions->length (), true);
why is carrying_args->jump_functions->length () the correct number of
jump functions? You need the lenght of the vector
callback_get_arg_mapping would return, no? (I'm not suggesting that we
construct such a vector here but I think the length needs to be computed
from the attribute and not from the number of parameters the dispatching
function has.)
You are right, that should be calculated from the attribute. It works
because the dispatching function in OpenMP/OpenACC usually takes more
arguments than the callback. The mistake looks to be isolated to this
place, I am using the correct number of jump functions in the rest of
the code. The doc comment above the function is pretty ironic though :)
Generally, this version is a clear improvement over the previous
versions and (with at least the last issue described above fixed), I'd
be happy to have the cgraph and ipa bits committed. Someone else needs
to approve the rest though - and Honza may want to chime in. I also
hope that soon (at Cauldron?) we'll be able to come to some kind of
conclusion what to do about the attribute name and behavior.
That's what I am hoping for. I'll be giving a short talk about this
patch and its future improvements at Cauldron on Sunday with the hopes
of coming up with at least some general direction for the attribute.
We may also consider enabling the new behavior with a compiler switch
(enabled by default at -O2 and higher). But that is also a detail.
I don't think that's currently needed, as the new edges are bound to
ipa-cp, which is enabled at -O2. Though that might change in the
future, we might add some functionality not specific to any pass.
Thanks a lot again for working on this.
Martin
Best regards,
Josef Melcr
