Hello,
On Sat, Sep 20 2025, Josef Melcr wrote:
> 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.
OH, right, I missed that. Make sense.
>
>>> + 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.
Please do. If it turns out to be necessary, we can bring it back.
>>> + /* 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.
I guess you are right. That is why I just wanted to document it
consistently.
>>> + /* 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.
I see. Because the patch does not touch ICF I thought this was some
left-over. But then I guess we need the field for all redirections (and
more) so singling out ICF sounds redundant.
>
>>> +/* 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 :)
It is a simple mistake to make. The comment made it easier to detect,
so it already was helpful.
>> 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.
I'm looking forward to it.
>> 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.
I don't have a strong opinion at the moment but it may turn out that it
we want it for debugging. I also suspect that at some point we will
want to have a debug counter check (see gcc/dbgcnt.h and description of
compiler option -fdbg-cnt in the user manual) somewhere too ;-)
Thanks a lot for working on this,
Martin
