On Sat, Jul 6, 2024 at 8:45 PM Andi Kleen <a...@linux.intel.com> wrote:
>
> > > if (!single_succ_p (bb))
> > > - return;
> > > + {
> > > + int num_eh, num_other;
> > > + bb_get_succ_edge_count (bb, num_eh, num_other);
> > > + /* Allow EH edges so that we can give a better
> > > + error message later. */
> >
> > Please instead use has_abnormal_or_eh_outgoing_edge_p (bb) instead
>
> That's not equivalent, need a num_other == 1 check too.
There can be at most one regular outgoing edge for a block with an
outgoing EH or abnormal edge.
> Do you want me to move the function to a generic place?
Maybe you can use find_fallthru_edge () instead if you think
has_abnormal_or_eh_outgoing_edge_p isn't good enough? That will
find the single_succ_edge when the BB isn't single_succ_p because
of EH/abnormal edges.
I think both choices would be equivalent to your new function and its use.
> > to avoid adding another function like this. Also only continue searching
> > for a musttail call if cfun->has_musttail
>
> Done (although I must say I liked the better dump messages even for non
> tailcall)
But this patch didn't add dumping (as said, squashing would have make
getting the whole picture easier).
> > > if (gimple_references_memory_p (stmt)
> > > || gimple_has_volatile_ops (stmt))
> > > - return;
> > > + {
> > > + bad_stmt = true;
> >
> > break here when !cfun->has_musttail?
>
> Done.
>
> > > if (ass_var
> > > && !is_gimple_reg (ass_var)
> > > && !auto_var_in_fn_p (ass_var, cfun->decl))
> > > - return;
> > > + {
> > > + maybe_error_musttail (call, _("return value in memory"));
> > > + return;
> > > + }
> > > +
> > > + if (cfun->calls_setjmp)
> > > + {
> > > + maybe_error_musttail (call, _("caller uses setjmp"));
> > > + return;
> > > + }
> > >
> > > /* If the call might throw an exception that wouldn't propagate out of
> > > cfun, we can't transform to a tail or sibling call (82081). */
> > > - if (stmt_could_throw_p (cfun, stmt)
> > > - && !stmt_can_throw_external (cfun, stmt))
> > > + if ((stmt_could_throw_p (cfun, stmt)
> > > + && !stmt_can_throw_external (cfun, stmt)) || !single_succ_p (bb))
> >
> > This reports for the found stmt while above we reject any intermediate
> > non-fallthru control flow. I would suggest to, in the above BB check,
> > record a gimple *last = last_stmt (bb) and if last == stmt report this
> > reason
> > but otherwise "control altering statement between call and return"?
>
> Ok. I reported "code between call and return". I don't think there
> since "control" would imply control flow.
>
> Also there is no last_stmt () or did I miss it? It couldn't be used
> anyways because it still needs to skip the nops etc. But the backwards
> loop can easily discover it.
Ah, yeah - you're now supposed to use *gsi_last_bb (bb)
> BTW I suspect some of the checks are redundant but it is hard to really
> prove it, so I left everything in place.
>
> > > + maybe_error_musttail (call,
> > > + _("call may throw exception that does not
> > > propagate"));
> > > return;
> > > + }
> > >
> > > /* If the function returns a value, then at present, the tail call
> > > must return the same type of value. There is conceptually a copy
> > > @@ -524,7 +593,10 @@ find_tail_calls (basic_block bb, struct tailcall
> > > **ret, bool only_musttail)
> > > if (result_decl
> > > && may_be_aliased (result_decl)
> > > && ref_maybe_used_by_stmt_p (call, result_decl, false))
> > > - return;
> > > + {
> > > + maybe_error_musttail (call, _("tail call must be same type"));
> >
> > ? "call uses the return slot"?
> >
> > Otherwise looks OK.
>
> Done. Although I'm not sure what a return slot is, but maybe the users
> can figure it out)
The comment above the check is a bit weird in how it talks about types, but
"tail call must be same type" isn't very helpful and it isn't in any way related
to the actual check being performed. "return slot" is supposed to be the
storage used for return pointed to by the invisible reference parameter to
space allocated by the caller. Do you know a more C/C++ standard related
naming for this?
Richard.
>
> -Andi
