[Bug tree-optimization/58253] New: IPA-SRA creates aligned loads from unaligned memory

[Bug tree-optimization/58253] New: IPA-SRA creates aligned loads from unaligned memory

[amylaar at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

            Bug ID: 58253
           Summary: IPA-SRA creates aligned loads from unaligned memory
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: amylaar at gcc dot gnu.org
                CC: amylaar at gcc dot gnu.org, jamborm at gcc dot gnu.org,
                    rguenth at gcc dot gnu.org
        Depends on: 52402
            Target: epiphany-*-*

+++ This bug was initially created as a clone of Bug #52402 +++

What I see on epiphany (at O2 and above, enabling SRA) for
gcc.dg/torture/pr52402.c is that the call of foo.isra.0 in main is done
using an aligned type:

Breakpoint 5, epiphany_function_arg (cum_v=..., mode=TImode, type=0xb7c5f960, 
    named=true)
    at /home/amylaar/adapteva/git/sdk/srcw/gcc/config/epiphany/epiphany.c:2177
2177      CUMULATIVE_ARGS cum = *get_cumulative_args (cum_v);
(gdb) call debug_tree(type)
 <vector_type 0xb7c5f960
    type <integer_type 0xb7bed360 int public SI
        size <integer_cst 0xb7bdd104 constant 32>
        unit size <integer_cst 0xb7bdd118 constant 4>
        align 32 symtab 0 alias set 3 canonical type 0xb7bed360 precision 32
min <integer_cst 0xb7bdd2f8 -2147483648> max <integer_cst 0xb7bdd30c
2147483647>
        pointer_to_this <pointer_type 0xb7beda80>>
    TI
    size <integer_cst 0xb7bdd460 type <integer_type 0xb7bed060 bitsizetype>
constant 128>
    unit size <integer_cst 0xb7bdd474 type <integer_type 0xb7bed000 sizetype>
constant 16>
    align 64 symtab 0 alias set 3 canonical type 0xb7c5f960 nunits 4 context
<translation_unit_decl 0xb7bf2e58 D.1323>
    pointer_to_this <pointer_type 0xb7c5fba0>>

but when the callee foo.isra.0 was compiled before, it got allegedly passed
an unaligned type:

Breakpoint 5, epiphany_function_arg (cum_v=..., mode=TImode, type=0xb7c6f300, 
    named=true)
    at /home/amylaar/adapteva/git/sdk/srcw/gcc/config/epiphany/epiphany.c:2177
2177      CUMULATIVE_ARGS cum = *get_cumulative_args (cum_v);
(gdb) call debug_tree(type)
 <vector_type 0xb7c6f300 v4si
    type <integer_type 0xb7bed360 int public SI
        size <integer_cst 0xb7bdd104 constant 32>
        unit size <integer_cst 0xb7bdd118 constant 4>
        align 32 symtab 0 alias set 3 canonical type 0xb7bed360 precision 32
min <integer_cst 0xb7bdd2f8 -2147483648> max <integer_cst 0xb7bdd30c
2147483647>
        pointer_to_this <pointer_type 0xb7beda80>>
    TI
    size <integer_cst 0xb7bdd460 type <integer_type 0xb7bed060 bitsizetype>
constant 128>
    unit size <integer_cst 0xb7bdd474 type <integer_type 0xb7bed000 sizetype>
constant 16>
    align 8 symtab 0 alias set -1 canonical type 0xb7c5f960 nunits 4 context
<translation_unit_decl 0xb7bf2e58 D.1323>>

When passing 64-bit aligned types, the epiphany ABI uses register pair(s)
to enable double load-store, thus in this case where r0 has already been
used, the argument starts at r2; in the less-aligned case, it starts at
r1.  Thus, the callee expects the argument in a different place than where
it has been put by the caller.

Richard Biener:

:So you say that on Epiphany the ABI of the function depends on the actual
:argument alignment?  And this alignment is taken from the _argument_, not
:the type of the function prototype?

Note, these are the types as passed to TARGET_FUNCTION_ARG.
The original function prototype is no longer in the picture because
the ipa optimizers have used a cloned/specialized function instead,
and the types used when constructing the call are not the same as when
generating the callee.

[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts

[jamborm at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #1 from Martin Jambor <jamborm at gcc dot gnu.org> ---
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  <mjambor@suse.cz>

    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);

[Bug tree-optimization/58253] IPA-SRA creates calls with different arguments that the callee accepts

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #2 from rguenther at suse dot de <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 <jamborm at gcc dot gnu.org> ---
> 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  <mjambor@suse.cz>
> 
>     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

[jamborm at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #3 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to rguenther@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

[jamborm at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to rguenther@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

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #5 from rguenther at suse dot de <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 <jamborm at gcc dot gnu.org> ---
> (In reply to rguenther@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

[amylaar at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #6 from Jorn Wolfgang Rennecke <amylaar at gcc dot gnu.org> ---
(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 ipa/58253] IPA-SRA creates calls with different arguments that the callee accepts

[jamborm at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-p
                   |                            |atches/2013-12/msg00252.htm
                   |                            |l
          Component|tree-optimization           |ipa

--- Comment #7 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Thanks I have posted the updated patch (which checks for
gimple_register_type rather than non-BLKmode) to the mailing list for
review:

http://gcc.gnu.org/ml/gcc-patches/2013-12/msg00252.html

Computing, storing and re-using the types would certainly be too
invasive a change for stage 3.  Moreover, it would basically mean
passing the PARM_DECL types as types of actual arguments and I am not
even sure that it is correct, the back-end should probably see the
actual arguments as exactly what they are in the callers.

[Bug ipa/58253] IPA-SRA creates calls with different arguments that the callee accepts

[amylaar at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #8 from Jorn Wolfgang Rennecke <amylaar at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #7)
> Thanks I have posted the updated patch (which checks for
> gimple_register_type rather than non-BLKmode)

FWIW, it is possible to have a BLKmode struct passed in a register.
The compat testcases have a number of those.  Not sure if it's
possible to craft a testcase that also triggers this ipa path.

> Computing, storing and re-using the types would certainly be too
> invasive a change for stage 3.  Moreover, it would basically mean
> passing the PARM_DECL types as types of actual arguments and I am not
> even sure that it is correct, the back-end should probably see the
> actual arguments as exactly what they are in the callers.

The idea of a function is that there can be multiple callers, using
different actual arguments, thus you shoud pick one formal argument type
for each argument, and stick with it for all callers and the callee.
The formal argument type determines how the argument is passed.
Now, I understand that with ipa, you will often have only a single
caller, and the compiler can change the types with consideration of the
passed actual arguments to fit various optimization purposes, but
it still has to pick one list of formal parameters types for each specialized
callee, and stick to this list at the corresponding call site(s).

[Bug ipa/58253] IPA-SRA creates calls with different arguments that the callee accepts

[jamborm at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58253

--- Comment #9 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Author: jamborm
Date: Thu Dec  5 18:07:08 2013
New Revision: 205715

URL: http://gcc.gnu.org/viewcvs?rev=205715&root=gcc&view=rev
Log:
2013-12-05  Martin Jambor  <mjambor@suse.cz>

    PR ipa/58253
    * ipa-prop.c (ipa_modify_formal_parameters): Create decls of
    non-BLKmode in their naturally aligned type.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-prop.c