On Thu, 13 Mar 2025, Jason Merrill wrote:
> On 3/13/25 3:54 AM, Richard Biener wrote:
> > On Thu, 13 Mar 2025, Jakub Jelinek wrote:
> >
> >> Hi!
> >>
> >> On Wed, Mar 12, 2025 at 02:01:14PM +0100, Richard Biener wrote:
> >>> On Wed, 12 Mar 2025, Jakub Jelinek wrote:
> >>>
> >>>> On Tue, Mar 11, 2025 at 12:13:13PM +0100, Richard Biener wrote:
> >>>>> On Tue, 11 Mar 2025, Jakub Jelinek wrote:
> >>>>>
> >>>>>> On Tue, Mar 11, 2025 at 10:18:18AM +0100, Richard Biener wrote:
> >>>>>>> I think the patch as-is is more robust, but still - ugh ... I wonder
> >>>>>>> whether we can instead avoid introducing the COMPLEX_EXPR at all
> >>>>>>> at -O0?
> >>>>>>
> >>>>>> Can we set DECL_NOT_GIMPLE_REG_P at -O0 during gimplification (where
> >>>>>> we've already handled some uses/setters of it), at least when
> >>>>>> gimplify_modify_expr_complex_part sees {REAL,IMAG}PART_EXPR on
> >>>>>> {VAR,PARM,RESULT}_DECL?
> >>>>>
> >>>>> Yes, that should work for LHS __real / __imag.
> >>>>
> >>>> Unfortunately it doesn't.
> >>>>
> >>>> Although successfully bootstrapped on x86_64-linux and i686-linux,
> >>>> it caused g++.dg/cpp1z/decomp2.C, g++.dg/torture/pr109262.C and
> >>>> g++.dg/torture/pr88149.C regressions.
> >>>>
> >>>> Minimal testcase is -O0:
> >>>> void
> >>>> foo (float x, float y)
> >>>> {
> >>>> __complex__ float z = x + y * 1.0fi;
> >>>> __real__ z = 1.0f;
> >>>> }
> >>>> which ICEs with
> >>>> pr88149.c: In function ‘foo’:
> >>>> pr88149.c:2:1: error: non-register as LHS of binary operation
> >>>> 2 | foo (float x, float y)
> >>>> | ^~~
> >>>> z = COMPLEX_EXPR <_2, y.0>;
> >>>> pr88149.c:2:1: internal compiler error: ‘verify_gimple’ failed
> >>>> When the initialization is being gimplified, z is still
> >>>> not DECL_NOT_GIMPLE_REG_P and so is_gimple_reg is true for it and
> >>>> so it gimplifies it as
> >>>> z = COMPLEX_EXPR <_2, y.0>;
> >>>> later, instead of building
> >>>> _3 = IMAGPART_EXPR <z>;
> >>>> z = COMPLEX_EXPR <1.0e+0, _3>;
> >>>> like before, the patch forces z to be not a gimple reg and uses
> >>>> REALPART_EXPR <z> = 1.0e+0;
> >>>> but it is too late, nothing fixes up the gimplification of the
> >>>> COMPLEX_EXPR
> >>>> anymore.
> >>>
> >>> Ah, yeah - setting DECL_NOT_GIMPLE_REG_P "after the fact" doesn't work.
> >>>
> >>>> So, I think we'd really need to do it the old way with adjusted naming
> >>>> of the flag, so assume for all non-addressable
> >>>> VAR_DECLs/PARM_DECLs/RESULT_DECLs with COMPLEX_TYPE if (!optimize) they
> >>>> are DECL_NOT_GIMPLE_REG_P (perhaps with the exception of
> >>>> get_internal_tmp_var), and at some point (what) if at all optimize that
> >>>> away if the partial accesses aren't done.
> >>>
> >>> We could of course do that in is_gimple_reg (), but I'm not sure if
> >>> all places that would need to check do so. Alternatively gimplify
> >>>
> >>> __real x = ..
> >>>
> >>> into
> >>>
> >>> tem[DECL_NOT_GIMPLE_REG_P] = x;
> >>> __real tem = ...;
> >>> x = tem;
> >>
> >> We can't do that, that again causes the undesirable copying of often
> >> uninitialized part(s).
> >>
> >>> when 'x' is a is_gimple_reg? Of course for -O0 this would be quite bad.
> >>> Likewise for your idea - where would we do this optimization when not
> >>> optimizing?
> >>>
> >>> So it would need to be the frontend(s) setting DECL_NOT_GIMPLE_REG_P
> >>> when producing lvalue __real/__imag accesses?
> >>
> >> The following patch sets it in the FEs during genericization.
> >> I think Fortran doesn't have a way to modify just real or just complex
> >> part separately.
> >>
> >> In short, this patch is for code like
> >> _ComplexT __t;
> >> __real__ __t = __z.real();
> >> __imag__ __t = __z.imag();
> >> _M_value *= __t;
> >> return *this;
> >> at -O0 which used to appear widely even in libstdc++ before GCC 9
> >> and happens in real-world code. At -O0 for debug info reasons (see
> >> PR119190) we don't want to aggressively DCE statements and when we
> >> since r0-100845 try to rewrite vars with COMPLEX_TYPE into SSA form
> >> aggressively, the above results in copying of uninitialized data
> >> when expanding COMPLEX_EXPRs added so that the vars can be in SSA form.
> >> The patch detects during genericization the partial initialization and
> >> doesn't rewrite such vars to SSA at -O0. This has to be done before
> >> gimplification starts, otherwise e.g. the attached testcase ICEs.
> >>
> >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > LGTM, please leave frontend maintainers a chance to comment though.
>
> No objection.
>
> Though I notice that the documentation of DECL_NOT_GIMPLE_REG_P seems
> backwards?
Oops - I'll fix that up.
Richard.
> Jason
>
>
>
--
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)
