diff mbox

[RFC] Fix SRA with respect of BIT_FIELD_REF

Message ID CA+=Sn1mrjY53EOnqQbqjxf2zG8_gm+DYd26kq2gjkSLqmdvY+w@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski June 2, 2012, 9:02 a.m. UTC 
On Fri, Jun 1, 2012 at 4:44 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Fri, Jun 01, 2012 at 11:31:20AM +0200, Richard Guenther wrote:
>> On Fri, Jun 1, 2012 at 6:02 AM, Andrew Pinski
>> <andrew.pinski@caviumnetworks.com> wrote:
>> > Hi,
>> >  When I modified GCC to change the majority of bitfield accesses
>> > which were done via component ref to BIT_FIELD_REF, SRA messes up
>> > because when it does the replacement it still tries to use the
>> > BIT_FIELD_REF except it never places the old value in the struct for
>> > the BIT_FIELD_REF to work correctly.
>> >
>> > This patch fixes the problem by expanding what BIT_FIELD_REF does
>> > internally for both writing and reading.  Note we can't use
>> > BIT_FIELD_REF directly on the left hand since we don't support partial
>> > writes yet (except for vector and complex types).
>> >
>> > This is only a RFC since I don't know a way to reproduce this bug on
>> > the trunk. I tested it on x86_64-linux-gnu with no regressions.
>>
>> I'd rather disqualify SRA of BIT_FIELD_REFs on the LHS for now.  My goal
>> was to enable SRA of bitfields using the DECL_BIT_FIELD_REPRESENTATIVE
>> work - something you go against with replacing them with BIT_FIELD_REFs.
>
> SRA of bit-fields works now, it is just rather simple and can't
> optimize the bit-field accesses as we probably should.  Therefore I am
> all for using DECL_BIT_FIELD_REPRESENTATIVEs and looked at those
> patches with interest, I'm just wondering why we'd want to do it for
> non-addressable structures only, which is something SRA would imply if
> this "lowering" was part of it.
>
> BIT_FIELD_REFs of non-vectors on the LHS are not much tested, I'm
> afraid. I it is quite possible it does not do the right thing.
> Nevertheless, making regions accessed through them unscalarizable
> might also be an option, if it does not induce significant penalties
> anywhere.

Here is a new patch which fixes BFR's in a much simpler way.  The
problem is SRA recognizes it needs to reread the replacement but it
forgets it needs to the write in the replacement right before the
action happens.

Thanks,
Andrew pinski


>
> Thanks,
>
> Martin
>
>>
>> You'd replace
>>
>>   x = a.b;
>>
>> with
>>
>>   tem = a.<representative for b>;
>>   x = BIT_FIELD_REF <tem, ....>;
>>
>> and for stores with a read-modify-write sequence, possibly adding
>> the bitfield-insert tree I proposed some time ago.  Replace
>>
>>  a.b = x;
>>
>> with
>>
>>  tem = a.<representative for b>
>>  tem = BIT_FIELD_EXPR <tem, x, ...>;
>>  a.<representative for b> = tem;
>>
>> and I'd do this replacement in SRA for now whenever it would decide to
>> SRA a bitfield.
>>
>> Richard.
>>
>> > Thanks,
>> > Andrew Pinski
>> >
>> > ChangeLog:
>> > * tree-sra.c (sra_modify_expr): Handle BIT_FIELD_REF specially if we
>> > are doing a replacement of the struct with one variable.
diff mbox

Patch

Index: tree-sra.c
===================================================================
--- tree-sra.c	(revision 188138)
+++ tree-sra.c	(working copy)
@@ -2602,10 +2602,20 @@  sra_modify_expr (tree *expr, gimple_stmt
       if (!useless_type_conversion_p (type, access->type))
 	{
 	  tree ref;
+	  gimple stmt;
 
 	  ref = build_ref_for_model (loc, access->base, access->offset, access,
 				     NULL, false);
 
+	  /* Reads and writes need at least the original access setup. */
+	  if (access->grp_partial_lhs)
+	    repl = force_gimple_operand_gsi (gsi, repl, true, NULL_TREE,
+					     true, GSI_SAME_STMT);
+	  stmt = gimple_build_assign (ref, repl);
+	  gimple_set_location (stmt, loc);
+	  gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+
+	  /* Writes read from the original access after the write has happened. */
 	  if (write)
 	    {
 	      gimple stmt;
@@ -2617,17 +2627,6 @@  sra_modify_expr (tree *expr, gimple_stmt
 	      gimple_set_location (stmt, loc);
 	      gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
 	    }
-	  else
-	    {
-	      gimple stmt;
-
-	      if (access->grp_partial_lhs)
-		repl = force_gimple_operand_gsi (gsi, repl, true, NULL_TREE,
-						 true, GSI_SAME_STMT);
-	      stmt = gimple_build_assign (ref, repl);
-	      gimple_set_location (stmt, loc);
-	      gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
-	    }
 	}
       else
 	*expr = repl;