On Sun, 14 Feb 2016, Eric Botcazou wrote:
> > No, but if there is none left why would you want to "fix" SRA?
>
> As expected, it seems that the make_ssa_name_fn kludge is not sufficient, so
> I'm proposing to disable the PR65310 one-liner for selected targets, using
> the
> function_arg_boundary hook, until after we have a clear way out of this mess.
>
> Here's a summary of the situation:
>
> targhooks.c:default_function_arg_boundary N
> aarch64/aarch64.c:aarch64_function_arg_boundary Y
> arm/arm.c:arm_function_arg_boundary N
> c6x/c6x.c:c6x_function_arg_boundary Y
> epiphany/epiphany.c:epiphany_function_arg_boundary Y
> frv/frv.c:frv_function_arg_boundary N
> i386/i386.c:ix86_function_arg_boundary N
> ia64/ia64.c:ia64_function_arg_boundary Y
> iq2000/iq2000.c:iq2000_function_arg_boundary Y
> m32c/m32c.c:m32c_function_arg_boundary N
> mcore/mcore.c:mcore_function_arg_boundary N
> mips/mips.c:mips_function_arg_boundary Y
> msp430/msp430.c:msp430_function_arg_boundary N
> nds32/nds32.c:nds32_function_arg_boundary Y
> pa/pa.c:pa_function_arg_boundary N
> rl78/rl78.c:rl78_function_arg_boundary N
> rs6000/rs6000.c:rs6000_function_arg_boundary Y (aggr, AIX/ELFv2)
> rx/rx.c:rx_function_arg_boundary Y
> sparc/sparc.c:sparc_function_arg_boundary Y (64-bit)
> tilegx/tilegx.c:tilegx_function_arg_boundary Y
> tilepro/tilepro.c:tilepro_function_arg_boundary Y
> xtensa/xtensa.c:xtensa_function_arg_boundary Y
>
> A 'Y' means that the return value of function_arg_boundary depends on the
> alignment of the type it is directly passed (e.g. not on its main variant).
> 'aggr' means for aggregate types only, the other modifiers are ABIs.
>
> So if we add a test based on function_arg_boundary, we'll effectively disable
> the PR65310 one-liner in some cases for the following targets:
> aarch64, c6x, epiphany, ia64, iq2000, mips, nds32, rs6000 (aggr), rx, sparc,
> tilegx, tilepro, xtensa
>
> If we additionally test STRICT_ALIGNMENT, the set of targets shrinks to:
> c6x, epiphany, ia64, iq2000, mips, nds32, sparc, tilegx, tilepro, xtensa
>
> MIPS being in both sets, this will fix PR68273 in both cases.
I agree with Jakub on this (obviously), still a comment on the patch:
> Index: tree-sra.c
> ===================================================================
> --- tree-sra.c (revision 233338)
> +++ tree-sra.c (working copy)
> @@ -1681,9 +1681,22 @@ build_ref_for_offset (location_t loc, tr
> misalign = (misalign + offset) & (align - 1);
> if (misalign != 0)
> align = (misalign & -misalign);
> - if (align != TYPE_ALIGN (exp_type))
> +
> + /* Misaligning a type is generally OK (if it's naturally aligned). */
> + if (align < TYPE_ALIGN (exp_type))
> exp_type = build_aligned_type (exp_type, align);
So you simply assume that exp_type is naturally aligned here. I think
you should test align < TYPE_ALIGN (TYPE_MAIN_VARIANT (exp_type)) here,
no?
> + /* Overaligning it can be problematic because of calling conventions. */
> + else if (align > TYPE_ALIGN (exp_type))
> + {
> + tree aligned_type = build_aligned_type (exp_type, align);
> + if (targetm.calls.function_arg_boundary (TYPE_MODE (aligned_type),
> + aligned_type)
> + == targetm.calls.function_arg_boundary (TYPE_MODE (exp_type),
> + exp_type))
And if you get enough supporters to apply this kind of workaround
I'd prefer it to be in build_aligned_type itself, basically
refusing to build over-aligned types. And I'd rather make this
controlled by an internal flag that backends should consciously
set (aka a target hook). The above is simply a bit too ugly IMHO
and looks incomplete(?), cannot even the cummulative args machinery
end up with type-align specifics or are you sure those can only
be triggered from function_arg_boundary?
Note the real issue is overaligned register types. I've tried
Index: tree-ssa.c
===================================================================
--- tree-ssa.c (revision 233369)
+++ tree-ssa.c (working copy)
@@ -936,6 +936,22 @@ verify_ssa (bool check_modified_stmt, bo
name, stmt, virtual_operand_p (name)))
goto err;
}
+
+ if (! SSA_NAME_VAR (name)
+ && TYPE_MAIN_VARIANT (TREE_TYPE (name)) != TREE_TYPE (name))
+ {
+ error ("type is not main variant");
+ goto err;
+ }
+
but that's not too happy (to verify the change below). But verifying
we don't have over-aligned (or under-aligned) registers would be a good
thing and I think all registers should have main-variant types.
So I'd rather nail down the case that still gets through with the
below patch using sth like the above (well, as said, adjust it
to check all SSA names and TYPE_ALING == TYPE_ALIGN (TYPE_MAIN_VARIANT
())).
At least it seems you have a testcase that reproduces the error
so you should be able to point to a specific pass doing the wreckage
and provide preprocessed source and a cross-compiler setup to
investigate.
> + exp_type = aligned_type;
> + }
> +
> mem_ref = fold_build2_loc (loc, MEM_REF, exp_type, base, off);
> REF_REVERSE_STORAGE_ORDER (mem_ref) = reverse;
> if (TREE_THIS_VOLATILE (prev_base))
> Index: tree-ssanames.c
> ===================================================================
> --- tree-ssanames.c (revision 233338)
> +++ tree-ssanames.c (working copy)
> @@ -286,7 +286,7 @@ make_ssa_name_fn (struct function *fn, t
>
> if (TYPE_P (var))
> {
> - TREE_TYPE (t) = TYPE_MAIN_VARIANT (var);
> + TREE_TYPE (t) = var;
> SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE);
> }
> else
Note that this change was desirable on its own as this is how I expect
types to end up in the middle-end after we fix LTO debug and thus
can free more of the "language specific" type pieces in the middle-end.
Richard.
