On Sat, Aug 5, 2023 at 11:54 PM David Malcolm via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Sun, 2023-08-06 at 02:28 +0530, Prathamesh Kulkarni via Gcc-patches
> wrote:
> > On Fri, 4 Aug 2023 at 23:28, Bradley Lucier via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
>
> Hi Bradley and Prathamesh...
>
> > >
> > > The patch at the end adds a warning when a tail/sibling call cannot
> > > be
> > > optimized for various reasons.
> > >
> > > I built and tested GCC with and without the patch with
> > > configuration
> > >
> > > Configured with: ../../gcc-mainline/configure --enable-languages=c
> > > --disable-multilib --prefix=/pkgs/gcc-mainline --disable-werror
> > >
> > > There were some changes in the test results, but I can't say that
> > > they
> > > look substantive:
> > >
>
> [...]
>
> > >
> > > to test the new warning. The warnings are of the form, e.g.,
> > >
> > > ../../../gcc-mainline/gcc/tree-vect-stmts.cc:11990:44: warning:
> > > cannot
> > > apply sibling-call optimization: callee required more stack slots
> > > than
> > > the caller [-Wdisabled-optimization]
> > >
> > > These are the number of times this warning was triggered building
> > > stage1:
> > >
> > > grep warning: build.log | grep sibling | sed 's/^.*://' | sort |
> > > uniq -c
> > > 259 callee required more stack slots than the caller
> > > [-Wdisabled-optimization]
> > > 43 callee returns a structure [-Wdisabled-optimization]
> > >
> > > If this patch is OK, someone else will need to commit it for me.
> > >
> > > Brad
> > >
> > > gcc/Changelog
> > >
> > > * calls.cc (maybe_complain_about_tail_call) Add warning
> > > when
> > > tail or sibling call cannot be optimized.
> > Hi Bradley,
> > I don't have comments on the patch, but a new warning will also
> > require a corresponding entry in doc/invoke.texi.
>
> To nitpick, this isn't a new warning; the patch is extending an
> existing warning. Looking at the existing entry for that warning I
> see:
>
> @opindex Wdisabled-optimization
> @opindex Wno-disabled-optimization
> @item -Wdisabled-optimization
> Warn if a requested optimization pass is disabled. This warning does
> not generally indicate that there is anything wrong with your code; it
> merely indicates that GCC's optimizers are unable to handle the code
> effectively. Often, the problem is that your code is too big or too
> complex; GCC refuses to optimize programs when the optimization
> itself is likely to take inordinate amounts of time.
>
> ...which arguably fits the new functionality. Though I don't know how
> the optimizer maintainers feel about it. Also, as we add more stuff to
> this warning, would users need more fine-grained control over which
> things for the optimizer to complain about? I'm not sure.
I don't think this specific case qualifies for -Wdisabled-optimization.
The diagnostic is for cases the user can control and was invented
for limits we put up for compile-time and memory-usage issues
where there exist --param XYZ to adjust limits.
It would be more appropriate to change this to
dump_printf_loc (MSG_MISSED_OPTIMIZATION, ...)
where this was designe to diagnose cases the compiler failed to
optimize for other reasons than running into some --param.
So, NAK.
Thanks,
Richard.
> >
> > Thanks,
> > Prathamesh
> > >
> > > diff --git a/gcc/calls.cc b/gcc/calls.cc
> > > index 1f3a6d5c450..b95c876fda8 100644
> > > --- a/gcc/calls.cc
> > > +++ b/gcc/calls.cc
> > > @@ -1242,10 +1242,12 @@ void
> > > maybe_complain_about_tail_call (tree call_expr, const char
> > > *reason)
> > > {
> > > gcc_assert (TREE_CODE (call_expr) == CALL_EXPR);
> > > - if (!CALL_EXPR_MUST_TAIL_CALL (call_expr))
> > > - return;
> > > -
> > > - error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s",
> > > reason);
> > > + if (CALL_EXPR_MUST_TAIL_CALL (call_expr))
> > > + error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s",
> > > reason);
>
> The existing code use error_at, passing it the location of the
> call_expr...
>
> > > + else if (flag_optimize_sibling_calls)
> > > + warning (OPT_Wdisabled_optimization,
> > > + "cannot apply sibling-call optimization: %s",
> > > reason);
>
> ...but the warning branch uses "warning", which implicitly uses the
> input_location global variable. Is the warning reported at the correct
> place? It's better to use warning_at and pass it the location at which
> the warning should be emitted.
>
> The patch doesn't add any test cases, but I imagine any such cases
> would be very target-dependent (did I add any to my libgccjit version
> of this way back when?)
>
> Thanks for the patch; hope this is constructive
> Dave
>
>
>
> > > + return;
> > > }
> > >
> > > /* Fill in ARGS_SIZE and ARGS array based on the parameters found
> > > in
> > >
> > >
> >
>
