[patch] Fix ICE on unaligned record field

[patch] Fix ICE on unaligned record field

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

Hi,

the attached Ada testcase triggers an assertion in the RTL expander for the 
address operator because the operator has been applied to a non-byte-aligned  
record field.  The problematic ADDR_EXPR is built by ipa_modify_call_arguments 
which has a hole when get_addr_base_and_unit_offset returns NULL_TREE: the 
variable offset case is handled but not the non-byte-aligned case, which can 
rountinely happen in Ada, hence the proposed fix.

Tested on x86_64-suse-linux, OK for the mainline?


2014-11-28  Eric Botcazou  <ebotcazou@adacore.com>

	* ipa-prop.c (ipa_modify_call_arguments): Properly deal with unaligned
	aggregate parameters passed by value.


2014-11-28  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/specs/pack12.ads: New test.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1767 bytes --]

Index: ipa-prop.c
===================================================================
--- ipa-prop.c	(revision 218133)
+++ ipa-prop.c	(working copy)
@@ -3939,6 +3939,39 @@ ipa_modify_call_arguments (struct cgraph
 	      /* Aggregate arguments can have non-invariant addresses.  */
 	      if (!base)
 		{
+		  /* ??? If the original aggregate is passed by value, it may
+		     be not aligned on a unit boundary, in which case taking
+		     directly its address below is forbidden.  Unfortunately
+		     get_addr_base_and_unit_offset doesn't differentiate its
+		     two failure modes so we need to get our hands dirty.  */
+		  if (!addrof)
+		    {
+		      tree offset;
+		      HOST_WIDE_INT bitsize, bitpos;
+		      machine_mode mode;
+		      int unsignedp, volatilep = 0;
+		      get_inner_reference (prev_base, &bitsize, &bitpos,
+					   &offset, &mode, &unsignedp,
+					   &volatilep, false);
+		      if (bitpos % BITS_PER_UNIT)
+			{
+			  tree tmp
+			    = create_tmp_reg (TREE_TYPE (prev_base), NULL);
+			  tree *argp
+			    = gimple_call_arg_ptr (stmt, adj->base_index);
+			  gimple tem = gimple_build_assign (tmp, prev_base);
+			  tree vuse = gimple_vuse (stmt);
+			  tree new_vdef = copy_ssa_name (vuse, tem);
+			  gimple_set_vuse (tem, vuse);
+			  gimple_set_vdef (tem, new_vdef);
+			  SET_USE (gimple_vuse_op (stmt), new_vdef);
+			  /* Insert the temporary ahead of every subsequent
+			     adjustment and replace the argument in the call
+			     in case it is referenced more than once.  */
+			  gsi_insert_after (&prev_gsi, tem, GSI_SAME_STMT);
+			  *argp = prev_base = tmp;
+			}
+		    }
 		  base = build_fold_addr_expr (prev_base);
 		  off = build_int_cst (adj->alias_ptr_type,
 				       adj->offset / BITS_PER_UNIT);

[-- Attachment #3: pack12.ads --]
[-- Type: text/x-adasrc, Size: 296 bytes --]

-- { dg-do compile }
-- { dg-options "-O2" }

package Pack12 is

  type Rec1 is record
    B : Boolean;
    N : Natural;
  end record;

  type Rec2 is record
    B : Boolean;
    R : Rec1;
  end record;
  pragma Pack (Rec2);

  type Rec3 is tagged record
    R : Rec2;
  end record;

end Pack12;

Re: [patch] Fix ICE on unaligned record field

[Richard Biener]

On Fri, Nov 28, 2014 at 5:20 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> the attached Ada testcase triggers an assertion in the RTL expander for the
> address operator because the operator has been applied to a non-byte-aligned
> record field.  The problematic ADDR_EXPR is built by ipa_modify_call_arguments
> which has a hole when get_addr_base_and_unit_offset returns NULL_TREE: the
> variable offset case is handled but not the non-byte-aligned case, which can
> rountinely happen in Ada, hence the proposed fix.
>
> Tested on x86_64-suse-linux, OK for the mainline?

Umm.  So you are doing a possibly aggregate copy here?  Or how
are we sure we are dealing with a register op only?  (the function
is always a twisted maze to me...)

That said - I suppose this is called from IPA-SRA?  In that case,
can't we please avoid doing the transform in the first place?

Thanks,
Richard.

>
> 2014-11-28  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * ipa-prop.c (ipa_modify_call_arguments): Properly deal with unaligned
>         aggregate parameters passed by value.
>
>
> 2014-11-28  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/specs/pack12.ads: New test.
>
>
> --
> Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[Martin Jambor]

Hi,

On Mon, Dec 01, 2014 at 12:00:14PM +0100, Richard Biener wrote:
> On Fri, Nov 28, 2014 at 5:20 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> > Hi,
> >
> > the attached Ada testcase triggers an assertion in the RTL expander for the
> > address operator because the operator has been applied to a non-byte-aligned
> > record field.  The problematic ADDR_EXPR is built by ipa_modify_call_arguments
> > which has a hole when get_addr_base_and_unit_offset returns NULL_TREE: the
> > variable offset case is handled but not the non-byte-aligned case, which can
> > rountinely happen in Ada, hence the proposed fix.
> >
> > Tested on x86_64-suse-linux, OK for the mainline?
> 
> Umm.  So you are doing a possibly aggregate copy here?  Or how
> are we sure we are dealing with a register op only?  (the function
> is always a twisted maze to me...)
> 
> That said - I suppose this is called from IPA-SRA?  In that case,
> can't we please avoid doing the transform in the first place?
> 

I suppose that could be done by something like the following, which I
have tested only very mildly so far, in particular I have not double
checked that get_inner_reference is cfun-agnostic.

Hope it helps,

Martin



2014-12-03  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (ipa_sra_check_caller_data): New type.
	(has_caller_p): Removed.
	(ipa_sra_check_caller): New function.
	(ipa_sra_preliminary_function_checks): Use it.

diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index f213c80..900f3c3 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -4977,13 +4977,54 @@ modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
   return cfg_changed;
 }
 
-/* If NODE has a caller, return true.  */
+/* Means of communication between ipa_sra_check_caller and
+   ipa_sra_preliminary_function_checks.  */
+
+struct ipa_sra_check_caller_data
+{
+  bool has_callers;
+  bool bad_arg_alignment;
+};
+
+/* If NODE has a caller, mark that fact in DATA which is pointer to
+   ipa_sra_check_caller_data.  Also check all aggregate arguments in all known
+   calls if they are unit aligned and if not, set the appropriate flag in DATA
+   too. */
 
 static bool
-has_caller_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
+ipa_sra_check_caller (struct cgraph_node *node, void *data)
 {
-  if (node->callers)
-    return true;
+  if (!node->callers)
+    return false;
+
+  struct ipa_sra_check_caller_data *iscc;
+  iscc = (struct ipa_sra_check_caller_data *) data;
+  iscc->has_callers = true;
+
+  for (cgraph_edge *cs = node->callers; cs; cs = cs->next_caller)
+    {
+      gimple call_stmt = cs->call_stmt;
+      unsigned count = gimple_call_num_args (call_stmt);
+      for (unsigned i = 0; i < count; i++)
+	{
+	  tree arg = gimple_call_arg (call_stmt, i);
+	  if (is_gimple_reg (arg))
+	      continue;
+
+	  tree offset;
+	  HOST_WIDE_INT bitsize, bitpos;
+	  machine_mode mode;
+	  int unsignedp, volatilep = 0;
+	  get_inner_reference (arg, &bitsize, &bitpos, &offset, &mode,
+			       &unsignedp, &volatilep, false);
+	  if (bitpos % BITS_PER_UNIT)
+	    {
+	      iscc->bad_arg_alignment = true;
+	      return true;
+	    }
+	}
+    }
+
   return false;
 }
 
@@ -5038,14 +5079,6 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
       return false;
     }
 
-  if (!node->call_for_symbol_thunks_and_aliases (has_caller_p, NULL, true))
-    {
-      if (dump_file)
-	fprintf (dump_file,
-		 "Function has no callers in this compilation unit.\n");
-      return false;
-    }
-
   if (cfun->stdarg)
     {
       if (dump_file)
@@ -5064,6 +5097,25 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
       return false;
     }
 
+  struct ipa_sra_check_caller_data iscc;
+  memset (&iscc, 0, sizeof(iscc));
+  node->call_for_symbol_thunks_and_aliases (ipa_sra_check_caller, &iscc, true);
+  if (!iscc.has_callers)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "Function has no callers in this compilation unit.\n");
+      return false;
+    }
+
+  if (iscc.bad_arg_alignment)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "A function call has an argument with non-unit alignemnt.\n");
+      return false;
+    }
+
   return true;
 }
 

Re: [patch] Fix ICE on unaligned record field

[Eric Botcazou]

> I suppose that could be done by something like the following, which I
> have tested only very mildly so far, in particular I have not double
> checked that get_inner_reference is cfun-agnostic.

Thanks, this works fine on the testcase and I believe that get_inner_reference 
is indeed cfun-agnostic (for example it's called from front-ends).

> 2014-12-03  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-sra.c (ipa_sra_check_caller_data): New type.
> 	(has_caller_p): Removed.
> 	(ipa_sra_check_caller): New function.
> 	(ipa_sra_preliminary_function_checks): Use it.

If Richard and you think it's the way to go, then fine by me.

-- 
Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[Richard Biener]

On Wed, Dec 3, 2014 at 3:02 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Mon, Dec 01, 2014 at 12:00:14PM +0100, Richard Biener wrote:
>> On Fri, Nov 28, 2014 at 5:20 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> > Hi,
>> >
>> > the attached Ada testcase triggers an assertion in the RTL expander for the
>> > address operator because the operator has been applied to a non-byte-aligned
>> > record field.  The problematic ADDR_EXPR is built by ipa_modify_call_arguments
>> > which has a hole when get_addr_base_and_unit_offset returns NULL_TREE: the
>> > variable offset case is handled but not the non-byte-aligned case, which can
>> > rountinely happen in Ada, hence the proposed fix.
>> >
>> > Tested on x86_64-suse-linux, OK for the mainline?
>>
>> Umm.  So you are doing a possibly aggregate copy here?  Or how
>> are we sure we are dealing with a register op only?  (the function
>> is always a twisted maze to me...)
>>
>> That said - I suppose this is called from IPA-SRA?  In that case,
>> can't we please avoid doing the transform in the first place?
>>
>
> I suppose that could be done by something like the following, which I
> have tested only very mildly so far, in particular I have not double
> checked that get_inner_reference is cfun-agnostic.
>
> Hope it helps,
>
> Martin
>
>
>
> 2014-12-03  Martin Jambor  <mjambor@suse.cz>
>
>         * tree-sra.c (ipa_sra_check_caller_data): New type.
>         (has_caller_p): Removed.
>         (ipa_sra_check_caller): New function.
>         (ipa_sra_preliminary_function_checks): Use it.
>
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index f213c80..900f3c3 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -4977,13 +4977,54 @@ modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
>    return cfg_changed;
>  }
>
> -/* If NODE has a caller, return true.  */
> +/* Means of communication between ipa_sra_check_caller and
> +   ipa_sra_preliminary_function_checks.  */
> +
> +struct ipa_sra_check_caller_data
> +{
> +  bool has_callers;
> +  bool bad_arg_alignment;
> +};
> +
> +/* If NODE has a caller, mark that fact in DATA which is pointer to
> +   ipa_sra_check_caller_data.  Also check all aggregate arguments in all known
> +   calls if they are unit aligned and if not, set the appropriate flag in DATA
> +   too. */
>
>  static bool
> -has_caller_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
> +ipa_sra_check_caller (struct cgraph_node *node, void *data)
>  {
> -  if (node->callers)
> -    return true;
> +  if (!node->callers)
> +    return false;
> +
> +  struct ipa_sra_check_caller_data *iscc;
> +  iscc = (struct ipa_sra_check_caller_data *) data;
> +  iscc->has_callers = true;
> +
> +  for (cgraph_edge *cs = node->callers; cs; cs = cs->next_caller)
> +    {
> +      gimple call_stmt = cs->call_stmt;
> +      unsigned count = gimple_call_num_args (call_stmt);
> +      for (unsigned i = 0; i < count; i++)
> +       {
> +         tree arg = gimple_call_arg (call_stmt, i);
> +         if (is_gimple_reg (arg))

!handled_component_p (arg)

would better match what you are checking below.

Note that I think the place of the check is unfortunate as you for example
will not remove the argument if it is unused.  In fact I'm not yet sure
what transform exactly we are disabling.  I am guessing we are
passing an aggregate by value that resides at a bit-aligned offset
of some outer object:

  foo (x.aggr);

and the function then does

foo (Aggr a)
{
  int i = a.foo;
...
}

thus use only a part of the aggregate.  Then IPA SRA would like to
pass x.aggr.foo instead of x.aggr and thus tries to materialize a
load from x.aggr.foo at all callers but fails to do that in a valid way.

Erics fix did, at all callers

  Aggr tem = x.aggr;
  foo (tem.foo);

?

While we should be able to simply do

  foo (BIT_FIELD_REF <x.aggr, .....>)

with the appropriate bit offset and size?  (if that's of register type
you need to do the load in a separate stmt of couse).

Thus similar to Erics fix but avoiding the aggregate copy.

But maybe I am not really understanding the issue (didn't look at the
testcase).

Richard.

> +             continue;
> +
> +         tree offset;
> +         HOST_WIDE_INT bitsize, bitpos;
> +         machine_mode mode;
> +         int unsignedp, volatilep = 0;
> +         get_inner_reference (arg, &bitsize, &bitpos, &offset, &mode,
> +                              &unsignedp, &volatilep, false);
> +         if (bitpos % BITS_PER_UNIT)
> +           {
> +             iscc->bad_arg_alignment = true;
> +             return true;
> +           }
> +       }
> +    }
> +
>    return false;
>  }
>
> @@ -5038,14 +5079,6 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
>        return false;
>      }
>
> -  if (!node->call_for_symbol_thunks_and_aliases (has_caller_p, NULL, true))
> -    {
> -      if (dump_file)
> -       fprintf (dump_file,
> -                "Function has no callers in this compilation unit.\n");
> -      return false;
> -    }
> -
>    if (cfun->stdarg)
>      {
>        if (dump_file)
> @@ -5064,6 +5097,25 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
>        return false;
>      }
>
> +  struct ipa_sra_check_caller_data iscc;
> +  memset (&iscc, 0, sizeof(iscc));
> +  node->call_for_symbol_thunks_and_aliases (ipa_sra_check_caller, &iscc, true);
> +  if (!iscc.has_callers)
> +    {
> +      if (dump_file)
> +       fprintf (dump_file,
> +                "Function has no callers in this compilation unit.\n");
> +      return false;
> +    }
> +
> +  if (iscc.bad_arg_alignment)
> +    {
> +      if (dump_file)
> +       fprintf (dump_file,
> +                "A function call has an argument with non-unit alignemnt.\n");
> +      return false;
> +    }
> +
>    return true;
>  }
>

Re: [patch] Fix ICE on unaligned record field

[Eric Botcazou]

> Note that I think the place of the check is unfortunate as you for example
> will not remove the argument if it is unused.  In fact I'm not yet sure
> what transform exactly we are disabling.  I am guessing we are
> passing an aggregate by value that resides at a bit-aligned offset
> of some outer object:
> 
>   foo (x.aggr);
> 
> and the function then does
> 
> foo (Aggr a)
> {
>   int i = a.foo;
> ...
> }
> 
> thus use only a part of the aggregate.  Then IPA SRA would like to
> pass x.aggr.foo instead of x.aggr and thus tries to materialize a
> load from x.aggr.foo at all callers but fails to do that in a valid way.

Right, it's the usual MEM_EXPR business creating ADDR_EXPRs out of nowhere and 
miserably failing on something not addressable.

> Erics fix did, at all callers
> 
>   Aggr tem = x.aggr;
>   foo (tem.foo);
> 
> ?

Yes, because the code wants to take &tem afterwards.

> While we should be able to simply do
> 
>   foo (BIT_FIELD_REF <x.aggr, .....>)
> 
> with the appropriate bit offset and size?  (if that's of register type
> you need to do the load in a separate stmt of couse).
> 
> Thus similar to Erics fix but avoiding the aggregate copy.

Yes, that should be doable, but I'm not sure it's worth the hassle.

-- 
Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[Richard Biener]

On Thu, Dec 11, 2014 at 10:52 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Note that I think the place of the check is unfortunate as you for example
>> will not remove the argument if it is unused.  In fact I'm not yet sure
>> what transform exactly we are disabling.  I am guessing we are
>> passing an aggregate by value that resides at a bit-aligned offset
>> of some outer object:
>>
>>   foo (x.aggr);
>>
>> and the function then does
>>
>> foo (Aggr a)
>> {
>>   int i = a.foo;
>> ...
>> }
>>
>> thus use only a part of the aggregate.  Then IPA SRA would like to
>> pass x.aggr.foo instead of x.aggr and thus tries to materialize a
>> load from x.aggr.foo at all callers but fails to do that in a valid way.
>
> Right, it's the usual MEM_EXPR business creating ADDR_EXPRs out of nowhere and
> miserably failing on something not addressable.

Well, I call it a convenience that MEM_EXPR, unlike INDIRECT_REF, can
be used to encapsulate an arbitrary byte-offset and view-conversion.  Of course
it's still a dereference of an address so that convenience doesn't work on sth
non-addressable.

>> Erics fix did, at all callers
>>
>>   Aggr tem = x.aggr;
>>   foo (tem.foo);
>>
>> ?
>
> Yes, because the code wants to take &tem afterwards.
>
>> While we should be able to simply do
>>
>>   foo (BIT_FIELD_REF <x.aggr, .....>)
>>
>> with the appropriate bit offset and size?  (if that's of register type
>> you need to do the load in a separate stmt of couse).
>>
>> Thus similar to Erics fix but avoiding the aggregate copy.
>
> Yes, that should be doable, but I'm not sure it's worth the hassle.

I'll leave that to you two to decide - Martins patch is ok if you are fine
with disabling the optimization (also removing an unused parameter).

Thanks,
Richard.

> --
> Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[Eric Botcazou]

> Well, I call it a convenience that MEM_EXPR, unlike INDIRECT_REF, can
> be used to encapsulate an arbitrary byte-offset and view-conversion.  Of
> course it's still a dereference of an address so that convenience doesn't
> work on sth non-addressable.

No discussion on the merits of MEM_EXPR vs INDIRECT_REF but on the pertinence 
of creating ADDR_EXPRs out of nowhere just to use them.

> I'll leave that to you two to decide - Martins patch is ok if you are fine
> with disabling the optimization (also removing an unused parameter).

I'm fine with disabling it: the aggregate is passed directly so it's probably 
small and, in the case at hand, the optimized caller would do 2 extractions 
instead of only 1 so the gain is not obvious.

-- 
Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[Eric Botcazou]

Martin,

> I suppose that could be done by something like the following, which I
> have tested only very mildly so far, in particular I have not double
> checked that get_inner_reference is cfun-agnostic.

The patch introduces no regressions on x86-64/Linux and makes the testcase 
(gnat.dg/specs/pack12.ads attached to the first message) pass.

Do you plan to install it (along with the testcase)?

> 2014-12-03  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-sra.c (ipa_sra_check_caller_data): New type.
> 	(has_caller_p): Removed.
> 	(ipa_sra_check_caller): New function.
> 	(ipa_sra_preliminary_function_checks): Use it.

-- 
Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[Martin Jambor]

Hi Eric and Richard,

On Tue, Jan 06, 2015 at 06:07:12PM +0100, Eric Botcazou wrote:
> Martin,
> 
> > I suppose that could be done by something like the following, which I
> > have tested only very mildly so far, in particular I have not double
> > checked that get_inner_reference is cfun-agnostic.
> 
> The patch introduces no regressions on x86-64/Linux and makes the testcase 
> (gnat.dg/specs/pack12.ads attached to the first message) pass.
> 
> Do you plan to install it (along with the testcase)?
> 

for various reasons I was not able to do it earlier, but today I have
re-bootstrapped the following (the only change is the added testcase)
on x86_64-linux and it passes OK.  Should I commit it to trunk then?

Thanks,

Martin


2015-02-25  Martin Jambor  <mjambor@suse.cz>
	    Eric Botcazou  <ebotcazou@adacore.com>

gcc/
	* tree-sra.c (ipa_sra_check_caller_data): New type.
	(has_caller_p): Removed.
	(ipa_sra_check_caller): New function.
	(ipa_sra_preliminary_function_checks): Use it.

gcc/changelog/
	* gnat.dg/specs/pack12.ads: New test.


diff --git a/gcc/testsuite/gnat.dg/specs/pack12.ads b/gcc/testsuite/gnat.dg/specs/pack12.ads
new file mode 100644
index 0000000..c5e962c
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/specs/pack12.ads
@@ -0,0 +1,21 @@
+-- { dg-do compile }
+-- { dg-options "-O2" }
+
+package Pack12 is
+
+  type Rec1 is record
+    B : Boolean;
+    N : Natural;
+  end record;
+
+  type Rec2 is record
+    B : Boolean;
+    R : Rec1;
+  end record;
+  pragma Pack (Rec2);
+
+  type Rec3 is tagged record
+    R : Rec2;
+  end record;
+
+end Pack12;
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 023b817..a6cddaf 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -5009,13 +5009,54 @@ modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
   return cfg_changed;
 }
 
-/* If NODE has a caller, return true.  */
+/* Means of communication between ipa_sra_check_caller and
+   ipa_sra_preliminary_function_checks.  */
+
+struct ipa_sra_check_caller_data
+{
+  bool has_callers;
+  bool bad_arg_alignment;
+};
+
+/* If NODE has a caller, mark that fact in DATA which is pointer to
+   ipa_sra_check_caller_data.  Also check all aggregate arguments in all known
+   calls if they are unit aligned and if not, set the appropriate flag in DATA
+   too. */
 
 static bool
-has_caller_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
+ipa_sra_check_caller (struct cgraph_node *node, void *data)
 {
-  if (node->callers)
-    return true;
+  if (!node->callers)
+    return false;
+
+  struct ipa_sra_check_caller_data *iscc;
+  iscc = (struct ipa_sra_check_caller_data *) data;
+  iscc->has_callers = true;
+
+  for (cgraph_edge *cs = node->callers; cs; cs = cs->next_caller)
+    {
+      gimple call_stmt = cs->call_stmt;
+      unsigned count = gimple_call_num_args (call_stmt);
+      for (unsigned i = 0; i < count; i++)
+	{
+	  tree arg = gimple_call_arg (call_stmt, i);
+	  if (is_gimple_reg (arg))
+	      continue;
+
+	  tree offset;
+	  HOST_WIDE_INT bitsize, bitpos;
+	  machine_mode mode;
+	  int unsignedp, volatilep = 0;
+	  get_inner_reference (arg, &bitsize, &bitpos, &offset, &mode,
+			       &unsignedp, &volatilep, false);
+	  if (bitpos % BITS_PER_UNIT)
+	    {
+	      iscc->bad_arg_alignment = true;
+	      return true;
+	    }
+	}
+    }
+
   return false;
 }
 
@@ -5070,14 +5111,6 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
       return false;
     }
 
-  if (!node->call_for_symbol_thunks_and_aliases (has_caller_p, NULL, true))
-    {
-      if (dump_file)
-	fprintf (dump_file,
-		 "Function has no callers in this compilation unit.\n");
-      return false;
-    }
-
   if (cfun->stdarg)
     {
       if (dump_file)
@@ -5096,6 +5129,25 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
       return false;
     }
 
+  struct ipa_sra_check_caller_data iscc;
+  memset (&iscc, 0, sizeof(iscc));
+  node->call_for_symbol_thunks_and_aliases (ipa_sra_check_caller, &iscc, true);
+  if (!iscc.has_callers)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "Function has no callers in this compilation unit.\n");
+      return false;
+    }
+
+  if (iscc.bad_arg_alignment)
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "A function call has an argument with non-unit alignemnt.\n");
+      return false;
+    }
+
   return true;
 }
 

Re: [patch] Fix ICE on unaligned record field

[Eric Botcazou]

> for various reasons I was not able to do it earlier, but today I have
> re-bootstrapped the following (the only change is the added testcase)
> on x86_64-linux and it passes OK.  Should I commit it to trunk then?

Yes, that would be kind of you, thanks in advance.

-- 
Eric Botcazou

Re: [patch] Fix ICE on unaligned record field

[H.J. Lu]

On Wed, Feb 25, 2015 at 7:43 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi Eric and Richard,
>
> On Tue, Jan 06, 2015 at 06:07:12PM +0100, Eric Botcazou wrote:
>> Martin,
>>
>> > I suppose that could be done by something like the following, which I
>> > have tested only very mildly so far, in particular I have not double
>> > checked that get_inner_reference is cfun-agnostic.
>>
>> The patch introduces no regressions on x86-64/Linux and makes the testcase
>> (gnat.dg/specs/pack12.ads attached to the first message) pass.
>>
>> Do you plan to install it (along with the testcase)?
>>
>
> for various reasons I was not able to do it earlier, but today I have
> re-bootstrapped the following (the only change is the added testcase)
> on x86_64-linux and it passes OK.  Should I commit it to trunk then?
>
> Thanks,
>
> Martin
>
>
> 2015-02-25  Martin Jambor  <mjambor@suse.cz>
>             Eric Botcazou  <ebotcazou@adacore.com>
>
> gcc/
>         * tree-sra.c (ipa_sra_check_caller_data): New type.
>         (has_caller_p): Removed.
>         (ipa_sra_check_caller): New function.
>         (ipa_sra_preliminary_function_checks): Use it.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65305

-- 
H.J.