[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253 --- Comment #6 from Jorn Wolfgang Rennecke --- (In reply to Martin Jambor from comment #1) > But again, I am not really sure what the semantics of alignment of > scalar PARM_DECL is. The relevance of various type properties will vary from target to target. The only safe way for the caller to receive the arguments as passed is to have caller and callee agree on the types passed. It would seem to me that computing the types once and then storing them somewhere, so that identical argument lists are used when procesing caller and callee, is the safest way to make argument lists agree. However, if you can make sure that you compute the same types in both places, I suppose that should work too. >From a performance point of view, alignment to the natural alignment of an integral mode is generally better than a lesser alignment, because it allows efficient loads / stores to stack slots, should any become necessary. > Nevertheless, can you please check if the patch > indeed fixes the bug? If so, I'll post it to the mailing list for > review/further discussion. Thanks. The patch gets rid of the gcc.dg/torture/pr52402.c execution failures. The only other difference observed with/without the patch is 8192 vs. 8173 tests being run in the libstdc++-v3 testsuite; the number of tests run there under Fedora 19/20 appears to vary from time to time independently of the compiler under test, so without running a statistically significant number of test runs (which would take a few months), I wouldn't draw any conclusion regarding the compiler from these differences.
[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253 --- Comment #5 from rguenther at suse dot de --- On Fri, 29 Nov 2013, jamborm at gcc dot gnu.org wrote: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253 > > --- Comment #3 from Martin Jambor --- > (In reply to rguent...@suse.de from comment #2) > > Isn't it easier to avoid building a type with different alignment > > in the first place? Or do this adjustment in SRA where the bug > > happens? It seems that when SRA representatives ->type is unaligned > > that this means, for by value passing, the value is accessed unaligned > > in the caller only. Thus turn_representatives_into_adjustments > > should go back to naturally aligned ->type for !by_ref params. > > > > That's what the first version of my patch did :-) The problem with it > is that the alignment of the type in adjustment is how > ipa_modify_call_arguments figures out it needs to build unaligned > loads in the caller and so those types need to stay unaligned and only > ipa_modify_formal_parameters needs to be taught to ignore that. ;) > BTW, now I wonder whether it would make more sense to check for > is_gimple_reg_type instead of non-BLKmode-ness with perhaps the latter > asserted. Arguably, the actual arguments are (formally well aligned) > SSA names because their type is a gimple register. Well - how an actual PARM_DECL passed by value ends up being aligned is up to the target, independent of whether it will be an SSA register or an aggregate. But we certainly should make the life of the targets easier by using natural alignment when inventing new pass-by-value PARM_DECLs. Richard.
[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253 --- Comment #4 from Martin Jambor --- (In reply to rguent...@suse.de from comment #2) > Isn't it easier to avoid building a type with different alignment > in the first place? Or do this adjustment in SRA where the bug > happens? It seems that when SRA representatives ->type is unaligned > that this means, for by value passing, the value is accessed unaligned > in the caller only. Thus turn_representatives_into_adjustments > should go back to naturally aligned ->type for !by_ref params. > That's what the first version of my patch did :-) The problem with it is that the alignment of the type in adjustment is how ipa_modify_call_arguments figures out it needs to build unaligned loads in the caller, so those types need to stay unaligned and only ipa_modify_formal_parameters needs to be taught to ignore that. BTW, now I wonder whether it would make more sense to check for is_gimple_reg_type instead of non-BLKmode-ness with perhaps the latter asserted. Arguably, the actual arguments are (formally well aligned) SSA names because their type is a gimple register.
[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253 --- Comment #3 from Martin Jambor --- (In reply to rguent...@suse.de from comment #2) > Isn't it easier to avoid building a type with different alignment > in the first place? Or do this adjustment in SRA where the bug > happens? It seems that when SRA representatives ->type is unaligned > that this means, for by value passing, the value is accessed unaligned > in the caller only. Thus turn_representatives_into_adjustments > should go back to naturally aligned ->type for !by_ref params. > That's what the first version of my patch did :-) The problem with it is that the alignment of the type in adjustment is how ipa_modify_call_arguments figures out it needs to build unaligned loads in the caller and so those types need to stay unaligned and only ipa_modify_formal_parameters needs to be taught to ignore that. BTW, now I wonder whether it would make more sense to check for is_gimple_reg_type instead of non-BLKmode-ness with perhaps the latter asserted. Arguably, the actual arguments are (formally well aligned) SSA names because their type is a gimple register.
[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253
--- Comment #2 from rguenther at suse dot de ---
On Thu, 28 Nov 2013, jamborm at gcc dot gnu.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253
>
> --- Comment #1 from Martin Jambor ---
> I do not quite understand why the port needs to look at alignments of
> scalars passed by value at all but I assume there is a reason.
>
> When it comes to the IPA-SRA created formal parameter and actual
> argument in the pr52402.c testcase, the following happens. The
> decision in which register pair the parameter is passed is made in
> epiphany_function_arg_boundary. This is reached in two different
> ways:
>
> 1. When compiling foo.isra.0, indirectly from call to
>targetm.calls.function_incoming_arg at function.c:2440. The type
>is the type of the PARM_DECL which is aligned to 8 bits.
>
> 2. When expanding the call in main, indirectly from call to
>targetm.calls.function_arg. The type passed is the type of the
>actual argument, which is an anonymous SSA name which has the
>natural alignment of the node which is 64.
>
> Thus, epiphany_function_arg_boundary erroneously comes to two
> different conclusions. Assuming there is nothing wrong with the
> above, we can fix the problem in IPA-SRA by the patch below which sets
> the type of the PARM_DECL to natural mode alignment (bootstrapped and
> tested and tested on x86_64-linux, the same on ppc64 is underway).
> But again, I am not really sure what the semantics of alignment of
> scalar PARM_DECL is. Nevertheless, can you please check if the patch
> indeed fixes the bug? If so, I'll post it to the mailing list for
> review/further discussion. Thanks.
>
>
> 2013-11-28 Martin Jambor
>
> PR ipa/58253
> * ipa-prop.c (ipa_modify_formal_parameters): Create decls of
> non-BLKmode in their naturally aligned type.
>
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 6bdb0df..a26b11a 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -3443,7 +3443,15 @@ ipa_modify_formal_parameters (tree fndecl,
> ipa_parm_adjustment_vec adjustments)
>if (adj->by_ref)
> ptype = build_pointer_type (adj->type);
>else
> -ptype = adj->type;
> +{
> + ptype = adj->type;
> + if (TYPE_MODE (ptype) != BLKmode)
> +{
> + unsigned malign = GET_MODE_ALIGNMENT (TYPE_MODE (ptype));
> + if (TYPE_ALIGN (ptype) < malign)
> +ptype = build_aligned_type (ptype, malign);
> +}
> +}
Isn't it easier to avoid building a type with different alignment
in the first place? Or do this adjustment in SRA where the bug
happens? It seems that when SRA representatives ->type is unaligned
that this means, for by value passing, the value is accessed unaligned
in the caller only. Thus turn_representatives_into_adjustments
should go back to naturally aligned ->type for !by_ref params.
Richard.
[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253
--- Comment #1 from Martin Jambor ---
I do not quite understand why the port needs to look at alignments of
scalars passed by value at all but I assume there is a reason.
When it comes to the IPA-SRA created formal parameter and actual
argument in the pr52402.c testcase, the following happens. The
decision in which register pair the parameter is passed is made in
epiphany_function_arg_boundary. This is reached in two different
ways:
1. When compiling foo.isra.0, indirectly from call to
targetm.calls.function_incoming_arg at function.c:2440. The type
is the type of the PARM_DECL which is aligned to 8 bits.
2. When expanding the call in main, indirectly from call to
targetm.calls.function_arg. The type passed is the type of the
actual argument, which is an anonymous SSA name which has the
natural alignment of the node which is 64.
Thus, epiphany_function_arg_boundary erroneously comes to two
different conclusions. Assuming there is nothing wrong with the
above, we can fix the problem in IPA-SRA by the patch below which sets
the type of the PARM_DECL to natural mode alignment (bootstrapped and
tested and tested on x86_64-linux, the same on ppc64 is underway).
But again, I am not really sure what the semantics of alignment of
scalar PARM_DECL is. Nevertheless, can you please check if the patch
indeed fixes the bug? If so, I'll post it to the mailing list for
review/further discussion. Thanks.
2013-11-28 Martin Jambor
PR ipa/58253
* ipa-prop.c (ipa_modify_formal_parameters): Create decls of
non-BLKmode in their naturally aligned type.
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 6bdb0df..a26b11a 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -3443,7 +3443,15 @@ ipa_modify_formal_parameters (tree fndecl,
ipa_parm_adjustment_vec adjustments)
if (adj->by_ref)
ptype = build_pointer_type (adj->type);
else
-ptype = adj->type;
+{
+ ptype = adj->type;
+ if (TYPE_MODE (ptype) != BLKmode)
+{
+ unsigned malign = GET_MODE_ALIGNMENT (TYPE_MODE (ptype));
+ if (TYPE_ALIGN (ptype) < malign)
+ptype = build_aligned_type (ptype, malign);
+}
+}
if (care_for_types)
new_arg_types = tree_cons (NULL_TREE, ptype, new_arg_types);
