On Thu, Mar 13, 2025 at 10:00:48AM -0400, 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?
Because it was DECL_GIMPLE_REG_P initially.
Guess we should fix.
Jakub
