diff mbox

Fix unaligned access when predictive commoning (PR 71083)

Message ID AM4PR0701MB2162871F21B7E25326B9DADEE41E0@AM4PR0701MB2162.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Aug. 11, 2016, 10:08 a.m. UTC 
On 08/11/16, Richard Biener wrote:
> 
> The patch looks mostly ok, but
> 
> +      else
> +       {
> +         boff >>= LOG2_BITS_PER_UNIT;
> +         boff += tree_to_uhwi (component_ref_field_offset (ref));
> +         coff = size_binop (MINUS_EXPR, coff, ssize_int (boff));
> 
> how can we be sure that component_ref_field_offset is an INTEGER_CST?
> At least Ada can have variably-length fields before a bitfield.  We'd
> need to apply component_ref_field_offset to off in that case.  Which
> makes me wonder if we can simply avoid the COMPONENT_REF path in
> a first iteration of the patch and always build a BIT_FIELD_REF?
> It should solve the correctness issues as well and be more applicable
> for branches.
> 

Oh yes, thanks for catching that!

If that information is true, that ought to go into the comment before
the if, that would certainly be an interesting comment :-)

Are there any test cases for this non-constant field offsets?

I see many checks if TREE_TYPE of
component_ref_field_offset is INTEGER_CST, but with very little
background why it could be otherwise.

I think we should simply fall back to the BIT_FIELD_REF in that case,
that would mean, the if should be something like:

tree offset = component_ref_field_offset (ref);
if (boff % BITS_PER_UNIT != 0
    || !tree_fits_uhwi_p (offset))

And yes, the correctness issue can certainly be solved with the
BIT_FIELD_REF alone.

So, as requested, here is a first iteration of my patch that always builds
a BIT_FIELD_REF, together with the test cases.


Boot-strap & regression testing was done on x86_64-pc-linux-gnu.
Is it OK for trunk (and active branches)?


Thanks
Bernd.
2016-08-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR tree-optimization/71083
	* tree-predcom.c (ref_at_iteration): Correctly align the
	reference type.

testsuite:
2016-08-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR tree-optimization/71083
	* gcc.c-torture/execute/pr71083.c: New test.
	* gnat.dg/loop_optimization23.adb: New test.
	* gnat.dg/loop_optimization23_pkg.ads: New test.
	* gnat.dg/loop_optimization23_pkg.adb: New test.

Comments

Richard Biener Aug. 11, 2016, 10:30 a.m. UTC | #1 
On Thu, 11 Aug 2016, Bernd Edlinger wrote:

> On 08/11/16, Richard Biener wrote:
> > 
> > The patch looks mostly ok, but
> > 
> > +      else
> > +       {
> > +         boff >>= LOG2_BITS_PER_UNIT;
> > +         boff += tree_to_uhwi (component_ref_field_offset (ref));
> > +         coff = size_binop (MINUS_EXPR, coff, ssize_int (boff));
> > 
> > how can we be sure that component_ref_field_offset is an INTEGER_CST?
> > At least Ada can have variably-length fields before a bitfield.  We'd
> > need to apply component_ref_field_offset to off in that case.  Which
> > makes me wonder if we can simply avoid the COMPONENT_REF path in
> > a first iteration of the patch and always build a BIT_FIELD_REF?
> > It should solve the correctness issues as well and be more applicable
> > for branches.
> > 
> 
> Oh yes, thanks for catching that!
> 
> If that information is true, that ought to go into the comment before
> the if, that would certainly be an interesting comment :-)
> 
> Are there any test cases for this non-constant field offsets?
> 
> I see many checks if TREE_TYPE of
> component_ref_field_offset is INTEGER_CST, but with very little
> background why it could be otherwise.
> 
> I think we should simply fall back to the BIT_FIELD_REF in that case,
> that would mean, the if should be something like:
> 
> tree offset = component_ref_field_offset (ref);
> if (boff % BITS_PER_UNIT != 0
>     || !tree_fits_uhwi_p (offset))
> 
> And yes, the correctness issue can certainly be solved with the
> BIT_FIELD_REF alone.
> 
> So, as requested, here is a first iteration of my patch that always builds
> a BIT_FIELD_REF, together with the test cases.
> 
> 
> Boot-strap & regression testing was done on x86_64-pc-linux-gnu.
> Is it OK for trunk (and active branches)?

Yes.

Thanks,
Richard.
diff mbox

Patch

Index: gcc/tree-predcom.c
===================================================================
--- gcc/tree-predcom.c	(revision 239193)
+++ gcc/tree-predcom.c	(working copy)
@@ -213,6 +213,7 @@  along with GCC; see the file COPYING3.
 #include "tree-scalar-evolution.h"
 #include "params.h"
 #include "tree-affine.h"
+#include "builtins.h"
 
 /* The maximum number of iterations between the considered memory
    references.  */
@@ -1381,6 +1382,8 @@  ref_at_iteration (data_reference_p dr, i
   addr = force_gimple_operand_1 (unshare_expr (addr), stmts,
 				 is_gimple_mem_ref_addr, NULL_TREE);
   tree alias_ptr = fold_convert (reference_alias_ptr_type (DR_REF (dr)), coff);
+  tree type = build_aligned_type (TREE_TYPE (DR_REF (dr)),
+				  get_object_alignment (DR_REF (dr)));
   /* While data-ref analysis punts on bit offsets it still handles
      bitfield accesses at byte boundaries.  Cope with that.  Note that
      we cannot simply re-apply the outer COMPONENT_REF because the
@@ -1392,12 +1395,11 @@  ref_at_iteration (data_reference_p dr, i
     {
       tree field = TREE_OPERAND (DR_REF (dr), 1);
       return build3 (BIT_FIELD_REF, TREE_TYPE (DR_REF (dr)),
-		     build2 (MEM_REF, DECL_BIT_FIELD_TYPE (field),
-			     addr, alias_ptr),
+		     build2 (MEM_REF, type, addr, alias_ptr),
 		     DECL_SIZE (field), bitsize_zero_node);
     }
   else
-    return fold_build2 (MEM_REF, TREE_TYPE (DR_REF (dr)), addr, alias_ptr);
+    return fold_build2 (MEM_REF, type, addr, alias_ptr);
 }
 
 /* Get the initialization expression for the INDEX-th temporary variable
Index: gcc/testsuite/gcc.c-torture/execute/pr71083.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/pr71083.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/pr71083.c	(working copy)
@@ -0,0 +1,43 @@ 
+struct lock_chain {
+  unsigned int irq_context: 2,
+    depth: 6,
+    base: 24;
+};
+
+__attribute__((noinline, noclone))
+struct lock_chain * foo (struct lock_chain *chain)
+{
+  int i;
+  for (i = 0; i < 100; i++)
+    {
+      chain[i+1].base = chain[i].base;
+    }
+  return chain;
+}
+
+struct lock_chain1 {
+  char x;
+  unsigned short base;
+} __attribute__((packed));
+
+__attribute__((noinline, noclone))
+struct lock_chain1 * bar (struct lock_chain1 *chain)
+{
+  int i;
+  for (i = 0; i < 100; i++)
+    {
+      chain[i+1].base = chain[i].base;
+    }
+  return chain;
+}
+
+struct lock_chain test [101];
+struct lock_chain1 test1 [101];
+
+int
+main ()
+{
+  foo (test);
+  bar (test1);
+  return 0;
+}
Index: gcc/testsuite/gnat.dg/loop_optimization23.adb
===================================================================
--- gcc/testsuite/gnat.dg/loop_optimization23.adb	(revision 0)
+++ gcc/testsuite/gnat.dg/loop_optimization23.adb	(working copy)
@@ -0,0 +1,14 @@ 
+-- { dg-do run }
+-- { dg-options "-O3" }
+-- PR tree-optimization/71083
+with Loop_Optimization23_Pkg;
+use Loop_Optimization23_Pkg;
+procedure Loop_Optimization23 is
+  Test : ArrayOfStructB;
+begin
+  Test (0).b.b := 9999;
+  Foo (Test);
+  if Test (100).b.b /= 9999 then
+    raise Program_Error;
+  end if;
+end;
Index: gcc/testsuite/gnat.dg/loop_optimization23_pkg.adb
===================================================================
--- gcc/testsuite/gnat.dg/loop_optimization23_pkg.adb	(revision 0)
+++ gcc/testsuite/gnat.dg/loop_optimization23_pkg.adb	(working copy)
@@ -0,0 +1,11 @@ 
+-- { dg-do compile }
+-- { dg-options "-O3" }
+-- PR tree-optimization/71083
+package body Loop_Optimization23_Pkg is
+  procedure Foo (X : in out ArrayOfStructB) is
+  begin
+    for K in 0..99 loop
+      X (K+1).b.b := X (K).b.b;
+    end loop;
+  end Foo;
+end Loop_Optimization23_Pkg;
Index: gcc/testsuite/gnat.dg/loop_optimization23_pkg.ads
===================================================================
--- gcc/testsuite/gnat.dg/loop_optimization23_pkg.ads	(revision 0)
+++ gcc/testsuite/gnat.dg/loop_optimization23_pkg.ads	(working copy)
@@ -0,0 +1,17 @@ 
+-- PR tree-optimization/71083
+package Loop_Optimization23_Pkg is
+  type Nibble is mod 2**4;
+  type Int24  is mod 2**24;
+  type StructA is record
+    a : Nibble;
+    b : Int24;
+  end record;
+  pragma Pack(StructA);
+  type StructB is record
+    a : Nibble;
+    b : StructA;
+  end record;
+  pragma Pack(StructB);
+  type ArrayOfStructB is array(0..100) of StructB;
+  procedure Foo (X : in out ArrayOfStructB);
+end Loop_Optimization23_Pkg;