On Tue, 27 May 2025, Thomas Schwinge wrote:
> Hi!
>
> On 2025-05-23T17:01:31+0200, Richard Biener <rguent...@suse.de> wrote:
> > Am 23.05.2025 um 16:49 schrieb Thomas Schwinge <tschwi...@baylibre.com>:
> >> This fell out of me looking into PR119835. This doesn't resolve the
> >> underlying
> >> issue, but instead of failing GIMPLE semantics verification just by chance
> >> in
> >> the 'GIMPLE pass: nrv' context, it makes the issue observable generally.
> >> (... thereby regressing a small number of offloading test cases where host
> >> vs.
> >> offload compilers disagree on 'aggregate_value_p' for functions that return
> >> aggregate types.)
> >>
> >> This cross-references just the three places in the code that I ran into;
> >> likely there are more?
> >>
> >> No regressions for powerpc64le-unknown-linux-gnu, x86_64-pc-linux-gnu
> >> bootstrap
> >> and 'make check' (without offloading configured).
> >
> > I think this is a step in the wrong direction in absence of quoting the
> > wrong thing that happens downstream when we violate this (an assert does
> > not qualify). ESP. When at the same time we allow the actual thing
> > returned to be a register (aka SSA name)
>
> ACK; you certainly understand GIMPLE and RTL expansion semantics better
> than I do. ;-)
>
> You're also implicitly telling me that
> "'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 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
diff --git a/gcc/tree-nrv.cc b/gcc/tree-nrv.cc
index 180ce39de4c..3a036bc2c82 100644
--- 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)
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)
