On 05/05/15 11:54, Richard Earnshaw wrote: > On 05/05/15 08:32, Jakub Jelinek wrote: >> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote: >>> So at least changing arm_needs_doubleword_align for non-aggregates would >>> likely not break anything that hasn't been broken already and would unbreak >>> the majority of cases. >> >> Attached (untested so far). It indeed changes code generated for >> over-aligned va_arg, but as I believe you can't properly pass those in the >> ... caller, this should just fix it so that va_arg handling matches the >> caller (and likewise for callees for named argument passing). >> >>> The following testcase shows that eipa_sra changes alignment even for the >>> aggregates. Change aligned (8) to aligned (4) to see another possibility. >> >> Actually I misread it, for the aggregates esra actually doesn't change >> anything, which is the reason why the testcase doesn't fail. >> The problem with the scalars is that esra first changed it to the >> over-aligned MEM_REFs and then later on eipa_sra used the types of the >> MEM_REFs created by esra. >> >> 2015-05-05 Jakub Jelinek <ja...@redhat.com> >> >> PR target/65956 >> * config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate >> types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type itself. >> >> * gcc.c-torture/execute/pr65956.c: New test. >> >> --- gcc/config/arm/arm.c.jj 2015-05-04 21:51:42.000000000 +0200 >> +++ gcc/config/arm/arm.c 2015-05-05 09:20:52.481693337 +0200 >> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG >> static bool >> arm_needs_doubleword_align (machine_mode mode, const_tree type) >> { >> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY >> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY)); >> + if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY) >> + return true; > > I don't think this is right (though I suspect the existing code has the > same problem). We should only look at mode if there is no type > information. The problem is that GCC has a nasty habit of assigning > real machine modes to things that are really BLKmode and we've run into > several cases where this has royally screwed things up. So for > consistency in the ARM back-end we are careful to only use mode when > type is NULL (=> it's a libcall). > >> + if (type == NULL_TREE) >> + return false; >> + if (AGGREGATE_TYPE_P (type)) >> + return TYPE_ALIGN (type) > PARM_BOUNDARY; >> + return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY; >> } >> > > > > It ought to be possible to re-order this, though, to > > static bool > arm_needs_doubleword_align (machine_mode mode, const_tree type) > { > - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY > - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY)); > + if (type != NULL_TREE) > + { > + if (AGGREGATE_TYPE_P (type)) > + return TYPE_ALIGN (type) > PARM_BOUNDARY; > + return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY; > + } > + return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY); > } > > > Either way, this would need careful cross-testing against an existing > compiler. >
It looks as though either patch would cause ABI incompatibility for typedef int alignedint __attribute__((aligned((8)))); int __attribute__((weak)) foo (int a, alignedint b) {return b;} void bar (alignedint x) { foo (1, x); } Where currently gcc uses r2 as the argument register for b in foo. R.