[PATCH, PR 43164, PR 43191] Do not scalarize structures with zero-size bit-fields as their last field

[Martin Jambor]

Hi,

On Thu, Mar 04, 2010 at 08:05:17AM -0800, Andrew Pinski wrote:
>
>
> Sent from my iPhone
>
> On Mar 4, 2010, at 3:16 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>
>> the patch below fixes both PR 43164 and PR 43191, which are both
>> caused by a zero-sized bit-field at the end of a record.  The
>> solution is rather crude, I simply disabled total scalarization
>> (attempts to turn all scalar components into separate variables so
>> that the original aggregate disappears) of aggregates containing such
>> things which means - this is the only way of how accesses to such
>> fields get created.
>
> Why not just ignore them when scalarizing? Since they are just there to 
> change the bit alignment of the next field.
>

That wahs the first thing I tried.  It does fix the ICE but
unfortunately the fact that there is a gap at the end of a structure
which is not part of any scalar replacement confuses
analyze_access_subtree.  It thinks that there are unscalarized data in
the aggregate and therefore sets the grp_unscalarized_data flag which
makes the loads from it not disappear and so total scalarization does
not have the intended effect, it only creates unnecessary variables
and statements.

Obviously this could be solved but some part of SRA would either have
to traverse both types and accesses or we would have to have a new
flag that is being checked everywhere or build_ref_for_offset would
have to be much more complex.  I don't think it's worth it and so I
will commit the proposed patch.

Thanks,

Martin



>
>>
>> I have tried not doing this and change build_ref_for_offset so that it
>> could cope with them but this would have to involve trying (usually in
>> vain) to look into sub-records preceeding the one into which one would
>> normally look, doing the same thing even in arrays of records while
>> being careful not to get out-of bounds and who knows what else.  These
>> weird zero-sized bit-fields simply violate too many properties that
>> normal fields have and make build_ref_for_offset overly complicated
>> and not for too much gain, I assume.  Therefore I have given this up
>> and will attempt it again only if I am persuaded it is a good idea.
>>
>> Bootstrapped and tested on x86_64-linux. OK for trunk?
>>
>> Thanks,
>>
>> Martin
>>
>>
>> 2010-03-03  Martin Jambor  <mjambor@suse.cz>
>>
>>    PR tree-optimization/43164
>>    PR tree-optimization/43191
>>    * tree-sra.c (type_consists_of_records_p): Reject records with
>>    zero-size bit-fields at the end.
>>
>>    * testsuite/gcc.c-torture/compile/pr43164.c: New test.
>>    * testsuite/gcc.c-torture/compile/pr43191.c: Likewise.
>>
>> Index: mine/gcc/testsuite/gcc.c-torture/compile/pr43164.c
>> ===================================================================
>> --- /dev/null
>> +++ mine/gcc/testsuite/gcc.c-torture/compile/pr43164.c
>> @@ -0,0 +1,16 @@
>> +struct S0
>> +{
>> +  unsigned char f0;
>> +  int:0;
>> +};
>> +
>> +struct S1
>> +{
>> +  struct S0 f0;
>> +};
>> +
>> +struct S1 func_34 (void)
>> +{
>> +  struct S1 l_221 = { { 1 } };
>> +  return l_221;
>> +}
>> Index: mine/gcc/testsuite/gcc.c-torture/compile/pr43191.c
>> ===================================================================
>> --- /dev/null
>> +++ mine/gcc/testsuite/gcc.c-torture/compile/pr43191.c
>> @@ -0,0 +1,46 @@
>> +struct S0
>> +{
>> +};
>> +
>> +struct S1
>> +{
>> +  unsigned f0:27;
>> +  const unsigned:0;
>> +};
>> +
>> +struct S2
>> +{
>> +  unsigned f2:1;
>> +};
>> +
>> +unsigned char g_4[1][8][3][1][1][1];
>> +unsigned char *g_17;
>> +unsigned char **g_16[1][10][7];
>> +
>> +struct S2 g_35 = {
>> +  0
>> +};
>> +
>> +struct S2 *g_34 = &g_35;
>> +
>> +struct S1 func_86 (unsigned char p_87, struct S2 **p_89)
>> +{
>> +  struct S1 l_92[6][8][1][1] = {
>> +    16143586
>> +  }
>> +  ;
>> +  return l_92[0][0][0][0];
>> +}
>> +
>> +void func_28 (struct S1 p_30, const struct S1 p_32)
>> +{
>> +}
>> +
>> +void func_70 (unsigned char p_72)
>> +{
>> +  unsigned char *const *l_93 = &g_17;
>> +  struct S2 **l_94;
>> +  unsigned char *const *l_97 = &g_17;
>> +  func_28 (func_86 (p_72, 0),
>> +           func_86 (p_72, &g_34));
>> +}
>> Index: mine/gcc/tree-sra.c
>> ===================================================================
>> --- mine.orig/gcc/tree-sra.c
>> +++ mine/gcc/tree-sra.c
>> @@ -802,13 +802,15 @@ create_access (tree expr, gimple stmt, b
>>
>>
>> /* Return true iff TYPE is a RECORD_TYPE with fields that are either of 
>> gimple
>> -   register types or (recursively) records with only these two kinds of
>> -   fields.  */
>> +   register types or (recursively) records with only these two kinds of 
>> fields.
>> +   It also returns false if any of these records has a zero-size field as 
>> its
>> +   last field.  */
>>
>> static bool
>> type_consists_of_records_p (tree type)
>> {
>>   tree fld;
>> +  bool last_fld_has_zero_size = false;
>>
>>   if (TREE_CODE (type) != RECORD_TYPE)
>>     return false;
>> @@ -821,7 +823,13 @@ type_consists_of_records_p (tree type)
>>    if (!is_gimple_reg_type (ft)
>>        && !type_consists_of_records_p (ft))
>>      return false;
>> +
>> +    last_fld_has_zero_size = tree_low_cst (DECL_SIZE (fld), 1) == 0;
>>       }
>> +
>> +  if (last_fld_has_zero_size)
>> +    return false;
>> +
>>   return true;
>> }
>>