On Wed, 28 May 2025, Thomas Schwinge wrote:
> Hi!
>
> On 2025-05-28T09:18:29+0200, Richard Biener <rguent...@suse.de> wrote:
> > On Tue, 27 May 2025, Thomas Schwinge wrote:
> >> "'GIMPLE_RETURN' vs. 'RESULT_DECL' if 'aggregate_value_p'" isn't actually
> >> a GIMPLE semantics invariant, thanks. I conclude that in case that this
> >> "invariant" is violated, that's not a problem for RTL expansion of
> >> 'GIMPLE_RETURN', which is then handled like all the other cases where
> >> "we are not returning the current function's RESULT_DECL".
> >>
> >> I'm not sure whether just disabling the 'assert' in
> >> 'gcc/tree-nrv.cc:pass_nrv::execute' is conceptually right (or may
> >> potentially drive that pass into an inconsistent state), and as we of
> >> course intend to eventually fix this issue properly (thanks for your
> >> ideas in PR119835!), so for now, I propose to simply
> >> "Disable 'pass_nrv' for offloading compilation [PR119835]", see attached.
> >> Any comments before I push that?
> >
> > I'm not sure you can disable this pass - it runs even at -O0
>
> No, runs only for 'optimize > 0'.
>
> (I guess you were looking at 'pass_return_slot', living in the same
> file.)
>
> > so parts
> > of it might be required for correctness, since some types cannot be
> > copied. Maybe RTL expansion will apply NRV if that's the case,
> > irrespective of whether the flag is set, but maybe not.
> >
> > I think a more appropriate solution would be to simply change
> > the assert as follows
>
> > --- a/gcc/tree-nrv.cc
> > +++ b/gcc/tree-nrv.cc
> > @@ -171,12 +171,12 @@ pass_nrv::execute (function *fun)
> >
> > if (greturn *return_stmt = dyn_cast <greturn *> (stmt))
> > {
> > - /* In a function with an aggregate return value, the
> > - gimplifier has changed all non-empty RETURN_EXPRs to
> > - return the RESULT_DECL. */
> > + /* In a function with an aggregate return value, if
> > + there is a return that does not return RESULT_DECL
> > + we cannot perform NRV optimizations. */
> > ret_val = gimple_return_retval (return_stmt);
> > - if (ret_val)
> > - gcc_assert (ret_val == result);
> > + if (ret_val && ret_val != result)
> > + return 0;
> > }
> > else if (gimple_has_lhs (stmt)
>
> Ah, right, in this scanning stage, no code transformations have been done
> yet, so we may still 'return 0;' (..., which then effectively also
> disables the pass).
>
> But, really also lose the check for non-offloading configurations, or do
> this defensive variant only '#ifdef ACCEL_COMPILER', as in the attached
> "Defuse 'RESULT_DECL' check in 'pass_nrv' for offloading compilation
> [PR119835]"?
Nah, I don't like #ifdef ACCEL_COMPILER sprinkled around. You could
amend the comment to mention that while gimplification tries to ensure
the result is the RESULT_DECL this breaks for offloading.
Richard.
>
> Grüße
> Thomas
>
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
