Re: [patch] Fix ICE on unaligned record field

[Richard Biener <richard.guenther@gmail.com>]

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