| Message ID | 5056495.Dz861ZsnJV@polaris |
|---|---|
| State | New |
| Headers | show |
On Sun, Feb 14, 2016 at 09:39:56PM +0100, 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. How does that help? Testcases have been posted multiple times that show that if targets look at type alignment of non-aggregate types, they have just broken argument passing, so conditionally reverting the tree-sra improvements can't help. Jakub
> How does that help? Testcases have been posted multiple times that show > that if targets look at type alignment of non-aggregate types, they have > just broken argument passing, so conditionally reverting the tree-sra > improvements can't help. Well, that has been the case for 2 decades for some of them and the compiler worked just fine, so I can easily see how disabling the tree-sra change can help in the short term. As for calling it an improvement, just look at the audit trail, it's a quick change papering over a deficiency in the IR...
On Sun, Feb 14, 2016 at 10:35:17PM +0100, Eric Botcazou wrote: > > How does that help? Testcases have been posted multiple times that show > > that if targets look at type alignment of non-aggregate types, they have > > just broken argument passing, so conditionally reverting the tree-sra > > improvements can't help. > > Well, that has been the case for 2 decades for some of them and the compiler > worked just fine, so I can easily see how disabling the tree-sra change can > help in the short term. As for calling it an improvement, just look at the > audit trail, it's a quick change papering over a deficiency in the IR... No, it is a major deficiency in the backends. Jakub
> No, it is a major deficiency in the backends.
Back-ends were obviously written with the natural alignment of types in mind
and were not prepared for overaligned non-aggregate types. Fixing MIPS will
not fix the other dozen and one can wonder, as was already mentioned by a few
other people, whether the proper fix is not in the middle-end instead, because
adding a dozen of TYPE_MAIN_VARIANT (...) in a dozen of back-ends is IMO not a
good example of robust engineering.
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); + /* 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)) + 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
> 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.