[PATCH 0/3] Fixing expansion of misaligned MEM_REFs on strict-alignment targets

[PATCH 0/3] Fixing expansion of misaligned MEM_REFs on strict-alignment targets

[Martin Jambor]

Hi,

this is another iteration of my effort to fix expansion of misaligned
memory accesses on strict-alignment platforms (which was suggested by
Richi in http://gcc.gnu.org/ml/gcc-patches/2011-08/msg00931.html, my
previous attempt was posted as
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01378.html).

Most importantly, I realized that the previous second patch was wrong
because store_field, which is called in the same code path, itself
calls store_bit_field just as I would.  This then lead to unnecessary
tripling of the code handling misalignment in some cases.  The only
problem with removing it was that this code path calls expand_normal
to expand the base of the MEM_REF and that means the third patch to
expand_expr_real_1 kicks in, extracts the value into a register which
is returned to expand_assignment, the existing code then writes the
value to a register but never stores it into memory.

I came to conclusion that in this case expand_assignment should expand
the MEM_REF itself rather passing it to expand_normal, just as it
already does when expanding naked MEM_REF and introduced a new second
patch doing just that.  Because very similar code would now be twice
in expand_assignment and once in expand_expr_real_1, I introduced a
new function expanding it in all of these three cases (more details on
that in the email with the actual patch).

I have successfully bootstrapped the first patch and all three patches
combined patch on x86_64-linux, i686-linux, ia64-linux (without Ada)
and sparc64-linux (without Java).  I will test the second on its own
too, I have just not done that yet.

Thanks in advance for any comments,

Martin

[PATCH 3/3] Misaligned MEM_REF reads

[Martin Jambor]

[-- Attachment #1: misaligned_read.diff --]
[-- Type: text/plain, Size: 4571 bytes --]

Hi,

this patch is very similar to the one I posted before
(http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01377.html) except that
it is now adjusted to sit on top of the new one before this and does
not ignore complex numbers.

There are more movmisalign_optab generations in this function.  There
is the TARGET_MEM_REF case which I intend to piggy-back on in the same
way like MEM_REF is handled in this patch once it leaves the RFC
stage.  Finally, movmisalign_optab is also generated in
VIEW_CONVERT_EXPR case but as far as I understand the code, misaligned
loads are already handled there (only perhaps we should use
SLOW_UNALIGNED_ACCESS instead of STRICT_ALIGNMENT there?).

The three patches passed bootstrap and testing on x86_64-linux,
i686-linux, usparc64-linux (without Java) and ia64-linux (without
Ada).  I propose to commit it after the previous patch passes review.

Thanks,

Martin


2012-03-09  Martin Jambor  <mjambor@suse.cz>

	* expr.c (expand_expr_real_1): handle misaligned scalar reads from
	memory through MEM_REFs by calling extract_bit_field.

	* testsuite/gcc.dg/misaligned-expand-1.c: New test.


Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -9404,21 +9404,27 @@ expand_expr_real_1 (tree exp, rtx target
 	set_mem_addr_space (temp, as);
 	align = get_object_or_type_alignment (exp);
 	if (mode != BLKmode
-	    && align < GET_MODE_ALIGNMENT (mode)
-	    /* If the target does not have special handling for unaligned
-	       loads of mode then it can use regular moves for them.  */
-	    && ((icode = optab_handler (movmisalign_optab, mode))
-		!= CODE_FOR_nothing))
+	    && align < GET_MODE_ALIGNMENT (mode))
 	  {
-	    struct expand_operand ops[2];
+	    if ((icode = optab_handler (movmisalign_optab, mode))
+		!= CODE_FOR_nothing)
+	      {
+		struct expand_operand ops[2];
 
-	    /* We've already validated the memory, and we're creating a
-	       new pseudo destination.  The predicates really can't fail,
-	       nor can the generator.  */
-	    create_output_operand (&ops[0], NULL_RTX, mode);
-	    create_fixed_operand (&ops[1], temp);
-	    expand_insn (icode, 2, ops);
-	    return ops[0].value;
+		/* We've already validated the memory, and we're creating a
+		   new pseudo destination.  The predicates really can't fail,
+		   nor can the generator.  */
+		create_output_operand (&ops[0], NULL_RTX, mode);
+		create_fixed_operand (&ops[1], temp);
+		expand_insn (icode, 2, ops);
+		return ops[0].value;
+	      }
+	    else if (SLOW_UNALIGNED_ACCESS (mode, align))
+	      temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
+					0, TYPE_UNSIGNED (TREE_TYPE (exp)),
+					true, (modifier == EXPAND_STACK_PARM
+					       ? NULL_RTX : target),
+					mode, mode);
 	  }
 	return temp;
       }
Index: src/gcc/testsuite/gcc.dg/misaligned-expand-1.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/misaligned-expand-1.c
@@ -0,0 +1,41 @@
+/* Test that expand can generate correct loads of misaligned data even on
+   strict alignment platforms.  */
+
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+extern void abort ();
+
+typedef unsigned int myint __attribute__((aligned(1)));
+
+unsigned int
+foo (myint *p)
+{
+  return *p;
+}
+
+#define cst 0xdeadbeef
+#define NUM 8
+
+struct blah
+{
+  char c;
+  myint i[NUM];
+};
+
+struct blah g;
+
+int
+main (int argc, char **argv)
+{
+  int i, k;
+  for (k = 0; k < NUM; k++)
+    {
+      g.i[k] = cst;
+      i = foo (&g.i[k]);
+
+      if (i != cst)
+	abort ();
+    }
+  return 0;
+}
Index: src/gcc/testsuite/gcc.dg/misaligned-expand-3.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/misaligned-expand-3.c
@@ -0,0 +1,43 @@
+/* Test that expand can generate correct stores to misaligned data of complex
+   type even on strict alignment platforms.  */
+
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+extern void abort ();
+
+typedef _Complex float mycmplx __attribute__((aligned(1)));
+
+void
+foo (mycmplx *p, float r, float i)
+{
+  __real__ *p = r;
+  __imag__ *p = i;
+}
+
+#define cvr 3.2f
+#define cvi 2.5f
+#define NUM 8
+
+struct blah
+{
+  char c;
+  mycmplx x[NUM];
+} __attribute__((packed));
+
+struct blah g;
+
+int
+main (int argc, char **argv)
+{
+  int k;
+
+  for (k = 0; k < NUM; k++)
+    {
+      foo (&g.x[k], cvr, cvi);
+      if (__real__ g.x[k] != cvr
+	  || __imag__ g.x[k] != cvi)
+	abort ();
+    }
+  return 0;
+}

[PATCH 2/3] Move MEM_REF expansion to a new function

[Martin Jambor]

[-- Attachment #1: unify_mem_ref_base_expansion.diff --]
[-- Type: text/plain, Size: 8348 bytes --]

Hi,

when we expand a misaligned MEM_REF on the LHS, we must not call the
code in expand_expr_real_1 if the subsequent patch is applied, because
the code generates code extracting the contents of the memory to a
register, which is of course bad if the intent is to write into that
memory.  Therefore expand_assignment should expand MEM_REFs itself,
just as it do when it encounters naked misaligned ones.

In order not to have nearly identical code twice in expand_assignment
and once more in expand_expr_real_1, I put it into a separate function
expand_mem_ref_to_mem_rtx (I'll be happy to change the name to
anything more correct or fitting).  Nevertheless, the existing code
pieces in expand_assignment and expand_expr_real_1 sre not exactly
identical, the differences are:

- expand_expr_real_1 handles a special case when the defining
  statement of the MEM_REF base is a BIT_AND_EXPR, expand_assignment
  does not.  The changelog introducing the change says "TER
  BIT_AND_EXPRs into MEM_REFs" which I suspect is a good thing for
  LHSs as well, so I kept the code.

- When expanding the base, the two functions differ in the
  expand_modifier they pass down to expand_expr.  expand_assignment
  uses EXPAND_NORMAL while expand_expr_real_1 passes EXPAND_SUM.
  According to the comment in expr.h the latter seemed more permissive
  and so I used that, even though I admit I do not really know what
  the implications of this modifier are.  Is it OK to use EXPAND_SUM
  also on a LHS?

- expand_expr_real_1 calls memory_address_addr_space twice, whereas
  expand_assignment replaces the first call with
  convert_memory_address_addr_space.  Looking at the two functions I
  thought it might be OK to call memory_address_addr_space (which
  itself calls convert_memory_address_addr_space) only once.
  But again, my expertise in this area is limited, I'll be happy to be
  shown I'm wrong here.

So far I have bootstrapped and tested this patch separately on
x86_64-linx and i686-linux.  Additionally, it has also passed
bootstrap and testing on usparc64-linux and ia64-linux.

Thanks in advance for any comments,

Martin


2012-03-09  Martin Jambor  <mjambor@suse.cz>

	* expr.c (expand_mem_ref_to_mem_rtx): New function.
	(expand_assignment): Call it when expanding a MEM_REF on the LHS.
	(expand_expr_real_1): Likewise.

Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -4565,6 +4565,47 @@ mem_ref_refers_to_non_mem_p (tree ref)
 	  && !MEM_P (DECL_RTL (base)));
 }
 
+/* Expand a MEM_REF referring an object in memory to a MEM RTX.  Any spacial
+   treatment of misalignment must be handled on top of the returned result.  */
+
+static rtx
+expand_mem_ref_to_mem_rtx (tree ref)
+{
+  enum machine_mode address_mode, mode = TYPE_MODE (TREE_TYPE (ref));
+  tree base = TREE_OPERAND (ref, 0);
+  addr_space_t as
+    = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (ref, 0))));
+  gimple def_stmt;
+  rtx mem, op0;
+
+  gcc_checking_assert (!mem_ref_refers_to_non_mem_p (ref));
+
+  address_mode = targetm.addr_space.address_mode (as);
+
+  if ((def_stmt = get_def_for_expr (base, BIT_AND_EXPR)))
+    {
+      tree mask = gimple_assign_rhs2 (def_stmt);
+      base = build2 (BIT_AND_EXPR, TREE_TYPE (base),
+		     gimple_assign_rhs1 (def_stmt), mask);
+      TREE_OPERAND (ref, 0) = base;
+    }
+  op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_SUM);
+  op0 = convert_memory_address_addr_space (address_mode, op0, as);
+  if (!integer_zerop (TREE_OPERAND (ref, 1)))
+    {
+      rtx off
+        = immed_double_int_const (mem_ref_offset (ref), address_mode);
+      op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
+    }
+  op0 = memory_address_addr_space (mode, op0, as);
+  mem = gen_rtx_MEM (mode, op0);
+  set_mem_attributes (mem, ref, 0);
+  set_mem_addr_space (mem, as);
+  if (TREE_THIS_VOLATILE (ref))
+    MEM_VOLATILE_P (mem) = 1;
+  return mem;
+}
+
 /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
    is true, try generating a nontemporal store.  */
 
@@ -4600,46 +4641,31 @@ expand_assignment (tree to, tree from, b
 	   != CODE_FOR_nothing)
 	  || SLOW_UNALIGNED_ACCESS (mode, align)))
     {
-      addr_space_t as
-	= TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0))));
       struct expand_operand ops[2];
-      enum machine_mode address_mode;
-      rtx reg, op0, mem;
+      rtx reg, mem;
 
       reg = expand_expr (from, NULL_RTX, VOIDmode, EXPAND_NORMAL);
       reg = force_not_mem (reg);
 
       if (TREE_CODE (to) == MEM_REF)
-	{
-	  tree base = TREE_OPERAND (to, 0);
-	  address_mode = targetm.addr_space.address_mode (as);
-	  op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
-	  op0 = convert_memory_address_addr_space (address_mode, op0, as);
-	  if (!integer_zerop (TREE_OPERAND (to, 1)))
-	    {
-	      rtx off
-		= immed_double_int_const (mem_ref_offset (to), address_mode);
-	      op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
-	    }
-	  op0 = memory_address_addr_space (mode, op0, as);
-	  mem = gen_rtx_MEM (mode, op0);
-	  set_mem_attributes (mem, to, 0);
-	  set_mem_addr_space (mem, as);
-	}
+	mem = expand_mem_ref_to_mem_rtx (to);
       else if (TREE_CODE (to) == TARGET_MEM_REF)
 	{
+	  addr_space_t as
+	    = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0))));
 	  struct mem_address addr;
+	  rtx op0;
 	  get_address_description (to, &addr);
 	  op0 = addr_for_mem_ref (&addr, as, true);
 	  op0 = memory_address_addr_space (mode, op0, as);
 	  mem = gen_rtx_MEM (mode, op0);
 	  set_mem_attributes (mem, to, 0);
 	  set_mem_addr_space (mem, as);
+	  if (TREE_THIS_VOLATILE (to))
+	    MEM_VOLATILE_P (mem) = 1;
 	}
       else
 	gcc_unreachable ();
-      if (TREE_THIS_VOLATILE (to))
-	MEM_VOLATILE_P (mem) = 1;
 
       if (icode != CODE_FOR_nothing)
 	{
@@ -4737,7 +4763,11 @@ expand_assignment (tree to, tree from, b
       else
 	{
 	  misalignp = false;
-	  to_rtx = expand_normal (tem);
+          if (TREE_CODE (tem) == MEM_REF
+              && !mem_ref_refers_to_non_mem_p (tem))
+            to_rtx = expand_mem_ref_to_mem_rtx (tem);
+          else
+	    to_rtx = expand_normal (tem);
 	}
 
       /* If the bitfield is volatile, we want to access it in the
@@ -9395,17 +9425,12 @@ expand_expr_real_1 (tree exp, rtx target
 
     case MEM_REF:
       {
-	addr_space_t as
-	  = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));
-	enum machine_mode address_mode;
-	tree base = TREE_OPERAND (exp, 0);
-	gimple def_stmt;
 	enum insn_code icode;
-	unsigned align;
 	/* Handle expansion of non-aliased memory with non-BLKmode.  That
 	   might end up in a register.  */
 	if (mem_ref_refers_to_non_mem_p (exp))
 	  {
+	    tree base = TREE_OPERAND (exp, 0);
 	    HOST_WIDE_INT offset = mem_ref_offset (exp).low;
 	    tree bit_offset;
 	    tree bftype;
@@ -9437,32 +9462,9 @@ expand_expr_real_1 (tree exp, rtx target
 					bit_offset),
 				target, tmode, modifier);
 	  }
-	address_mode = targetm.addr_space.address_mode (as);
-	base = TREE_OPERAND (exp, 0);
-	if ((def_stmt = get_def_for_expr (base, BIT_AND_EXPR)))
-	  {
-	    tree mask = gimple_assign_rhs2 (def_stmt);
-	    base = build2 (BIT_AND_EXPR, TREE_TYPE (base),
-			   gimple_assign_rhs1 (def_stmt), mask);
-	    TREE_OPERAND (exp, 0) = base;
-	  }
-	align = get_object_or_type_alignment (exp);
-	op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_SUM);
-	op0 = memory_address_addr_space (address_mode, op0, as);
-	if (!integer_zerop (TREE_OPERAND (exp, 1)))
-	  {
-	    rtx off
-	      = immed_double_int_const (mem_ref_offset (exp), address_mode);
-	    op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
-	  }
-	op0 = memory_address_addr_space (mode, op0, as);
-	temp = gen_rtx_MEM (mode, op0);
-	set_mem_attributes (temp, exp, 0);
-	set_mem_addr_space (temp, as);
-	if (TREE_THIS_VOLATILE (exp))
-	  MEM_VOLATILE_P (temp) = 1;
+	temp = expand_mem_ref_to_mem_rtx (exp);
 	if (mode != BLKmode
-	    && align < GET_MODE_ALIGNMENT (mode)
+	    && get_object_or_type_alignment (exp) < GET_MODE_ALIGNMENT (mode)
 	    /* If the target does not have special handling for unaligned
 	       loads of mode then it can use regular moves for them.  */
 	    && ((icode = optab_handler (movmisalign_optab, mode))

[PATCH 1/3] Misaligned top level MEM_REFs on LHS of assignments

[Martin Jambor]

[-- Attachment #1: misaligned_write-first.diff --]
[-- Type: text/plain, Size: 2922 bytes --]

Hi,

this patch has not changed since I posted it the last time
(http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01375.html) except for a
fixed formatting of a comment.  It has been already approved by Richi
(http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01389.html) and I intend
to commit it this week even if the other patches still undergo review.

Thanks,

Martin

2012-03-08  Martin Jambor  <mjambor@suse.cz>

	* expr.c (expand_assignment): Handle misaligned scalar writes to
	memory through top-level MEM_REFs by calling store_bit_field.

	* testsuite/gcc.dg/misaligned-expand-2.c: New test.


Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -4593,10 +4593,12 @@ expand_assignment (tree to, tree from, b
   if ((TREE_CODE (to) == MEM_REF
        || TREE_CODE (to) == TARGET_MEM_REF)
       && mode != BLKmode
+      && !mem_ref_refers_to_non_mem_p (to)
       && ((align = get_object_or_type_alignment (to))
 	  < GET_MODE_ALIGNMENT (mode))
-      && ((icode = optab_handler (movmisalign_optab, mode))
-	  != CODE_FOR_nothing))
+      && (((icode = optab_handler (movmisalign_optab, mode))
+	   != CODE_FOR_nothing)
+	  || SLOW_UNALIGNED_ACCESS (mode, align)))
     {
       addr_space_t as
 	= TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0))));
@@ -4639,11 +4641,17 @@ expand_assignment (tree to, tree from, b
       if (TREE_THIS_VOLATILE (to))
 	MEM_VOLATILE_P (mem) = 1;
 
-      create_fixed_operand (&ops[0], mem);
-      create_input_operand (&ops[1], reg, mode);
-      /* The movmisalign<mode> pattern cannot fail, else the assignment would
-	 silently be omitted.  */
-      expand_insn (icode, 2, ops);
+      if (icode != CODE_FOR_nothing)
+	{
+	  create_fixed_operand (&ops[0], mem);
+	  create_input_operand (&ops[1], reg, mode);
+	  /* The movmisalign<mode> pattern cannot fail, else the assignment
+	     would silently be omitted.  */
+	  expand_insn (icode, 2, ops);
+	}
+      else
+	store_bit_field (mem, GET_MODE_BITSIZE (mode),
+			 0, 0, 0, mode, reg);
       return;
     }
 
Index: src/gcc/testsuite/gcc.dg/misaligned-expand-2.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/misaligned-expand-2.c
@@ -0,0 +1,42 @@
+/* Test that expand can generate correct stores to misaligned data even on
+   strict alignment platforms.  */
+
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+extern void abort ();
+
+typedef unsigned int myint __attribute__((aligned(1)));
+
+void
+foo (myint *p, unsigned int i)
+{
+  *p = i;
+}
+
+#define cst 0xdeadbeef
+#define NUM 8
+
+struct blah
+{
+  char c;
+  myint i[NUM];
+};
+
+struct blah g;
+
+#define cst 0xdeadbeef
+
+int
+main (int argc, char **argv)
+{
+  int k;
+
+  for (k = 0; k < NUM; k++)
+    {
+      foo (&g.i[k], cst);
+      if (g.i[k] != cst)
+	abort ();
+    }
+  return 0;
+}

Re: [PATCH 2/3] Move MEM_REF expansion to a new function

[Richard Guenther]

On Mon, 12 Mar 2012, Martin Jambor wrote:

> Hi,
> 
> when we expand a misaligned MEM_REF on the LHS, we must not call the
> code in expand_expr_real_1 if the subsequent patch is applied, because
> the code generates code extracting the contents of the memory to a
> register, which is of course bad if the intent is to write into that
> memory.  Therefore expand_assignment should expand MEM_REFs itself,
> just as it do when it encounters naked misaligned ones.

Just a quick comment here - the expand_expr_real_1 code needs
to be guarded with exactly the same conditions as the misaligned
LHS case to be able to call expand_expr on it and generate a
naked MEM.  So if that is not working you have a bug in the RHS
side handling ;)

Richard.

Re: [PATCH 2/3] Move MEM_REF expansion to a new function

[Michael Matz]

Hi,

On Mon, 12 Mar 2012, Martin Jambor wrote:

> when we expand a misaligned MEM_REF on the LHS, we must not call the
> code in expand_expr_real_1 if the subsequent patch is applied, because
> the code generates code extracting the contents of the memory to a
> register, which is of course bad if the intent is to write into that
> memory.

Then expand_expr_real_1 should be called with EXPAND_WRITE modifier, 
instead of any of the others.  Then it will (or should) return an lvalue.  
That might still be wrong for alignment reasons, but writing into the so 
returned rtx will change the original object.

> Therefore expand_assignment should expand MEM_REFs itself,
> just as it do when it encounters naked misaligned ones.

I think this goes into the wrong direction.  expand_assignment shouldn't 
create an lvalue rtx for any REFs itself.  It should call expand_expr with 
EXPAND_WRITE, and that should do the right thing (i.e. what's now done 
directly in expand_assignment).

I realize the docu of EXPAND_WRITE is lacking, but here's what I think it 
should do (and what I think it actually also mostly does already): Given 
EXPAND_WRITE expand_expr is required to be called on an (sub)object, i.e. 
an lvalue, and it should return an RTX lvalue (a REG or MEM) that if 
written into is changing the originally specified tree lvalue (i.e. not 
some temporary storage).

That doesn't mean that the result of expand_expr(EXPAND_WRITE) is directly 
usable in a simple RTL (set) pattern as LHS in all cases.  For instance it 
won't be directly usable when it's misalign.  Dealing with this situation 
is left to the caller, i.e. expand_assignment mostly.

> - When expanding the base, the two functions differ in the
>   expand_modifier they pass down to expand_expr.  expand_assignment
>   uses EXPAND_NORMAL while expand_expr_real_1 passes EXPAND_SUM.
>   According to the comment in expr.h the latter seemed more permissive
>   and so I used that, even though I admit I do not really know what
>   the implications of this modifier are.  Is it OK to use EXPAND_SUM
>   also on a LHS?

No, but it might not matter in the situations you are facing, haven't 
checked.  EXPAND_SUM can return a PLUS rtx, e.g.
  (plus (p60) (const_int 4))
for an offsetted address.  Naturally you can't assign into such a plus 
rtx.  But if you only expand the base with that modifier (which for 
BLKmode bases actually means expanding to a MEM containing the address of 
base) you should be fine with EXPAND_SUM, it won't be used I think.


Ciao,
Michael.

Re: [PATCH 2/3] Move MEM_REF expansion to a new function

[Martin Jambor]

Hi,

On Mon, Mar 12, 2012 at 04:26:21PM +0100, Michael Matz wrote:
> Hi,
> 
> On Mon, 12 Mar 2012, Martin Jambor wrote:
> 
> > when we expand a misaligned MEM_REF on the LHS, we must not call the
> > code in expand_expr_real_1 if the subsequent patch is applied, because
> > the code generates code extracting the contents of the memory to a
> > register, which is of course bad if the intent is to write into that
> > memory.
> 
> Then expand_expr_real_1 should be called with EXPAND_WRITE modifier, 
> instead of any of the others.  Then it will (or should) return an lvalue.  
> That might still be wrong for alignment reasons, but writing into the so 
> returned rtx will change the original object.

OK, the following patch changes the places where I previously called
the new function to call expand_expr with EXPAND_WRITE modifier and
then makes sure we do not perform reads into rvalues in
expand_expr_real_1 in the contexts where I need to avoid that.

So far it has passed bootstrap and testing on x86_64-linux, bootstraps
and testsuite runs on the other platforms are still underway.  What do
you think?

Thanks,

Martin


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

	* expr.c (expand_assignment): Use expand_expr with EXPAND_WRITE
	when expanding MEM_REFs, MEM_TARGET_REFs and handled_component
	bases.
	(expand_expr_real_1): Do not handle misalignment if modifier is
	EXPAND_WRITE.

Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -4600,49 +4600,16 @@ expand_assignment (tree to, tree from, b
 	   != CODE_FOR_nothing)
 	  || SLOW_UNALIGNED_ACCESS (mode, align)))
     {
-      addr_space_t as
-	= TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0))));
-      struct expand_operand ops[2];
-      enum machine_mode address_mode;
-      rtx reg, op0, mem;
+      rtx reg, mem;
 
       reg = expand_expr (from, NULL_RTX, VOIDmode, EXPAND_NORMAL);
       reg = force_not_mem (reg);
-
-      if (TREE_CODE (to) == MEM_REF)
-	{
-	  tree base = TREE_OPERAND (to, 0);
-	  address_mode = targetm.addr_space.address_mode (as);
-	  op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
-	  op0 = convert_memory_address_addr_space (address_mode, op0, as);
-	  if (!integer_zerop (TREE_OPERAND (to, 1)))
-	    {
-	      rtx off
-		= immed_double_int_const (mem_ref_offset (to), address_mode);
-	      op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
-	    }
-	  op0 = memory_address_addr_space (mode, op0, as);
-	  mem = gen_rtx_MEM (mode, op0);
-	  set_mem_attributes (mem, to, 0);
-	  set_mem_addr_space (mem, as);
-	}
-      else if (TREE_CODE (to) == TARGET_MEM_REF)
-	{
-	  struct mem_address addr;
-	  get_address_description (to, &addr);
-	  op0 = addr_for_mem_ref (&addr, as, true);
-	  op0 = memory_address_addr_space (mode, op0, as);
-	  mem = gen_rtx_MEM (mode, op0);
-	  set_mem_attributes (mem, to, 0);
-	  set_mem_addr_space (mem, as);
-	}
-      else
-	gcc_unreachable ();
-      if (TREE_THIS_VOLATILE (to))
-	MEM_VOLATILE_P (mem) = 1;
+      mem = expand_expr (to, NULL_RTX, VOIDmode, EXPAND_WRITE);
 
       if (icode != CODE_FOR_nothing)
 	{
+	  struct expand_operand ops[2];
+
 	  create_fixed_operand (&ops[0], mem);
 	  create_input_operand (&ops[1], reg, mode);
 	  /* The movmisalign<mode> pattern cannot fail, else the assignment
@@ -4695,31 +4662,11 @@ expand_assignment (tree to, tree from, b
 	  && ((icode = optab_handler (movmisalign_optab, mode))
 	      != CODE_FOR_nothing))
 	{
-	  enum machine_mode address_mode;
-	  rtx op0;
 	  struct expand_operand ops[2];
-	  addr_space_t as = TYPE_ADDR_SPACE
-	      (TREE_TYPE (TREE_TYPE (TREE_OPERAND (tem, 0))));
-	  tree base = TREE_OPERAND (tem, 0);
 
 	  misalignp = true;
 	  to_rtx = gen_reg_rtx (mode);
-
-	  address_mode = targetm.addr_space.address_mode (as);
-	  op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
-	  op0 = convert_memory_address_addr_space (address_mode, op0, as);
-	  if (!integer_zerop (TREE_OPERAND (tem, 1)))
-	    {
-	      rtx off = immed_double_int_const (mem_ref_offset (tem),
-						address_mode);
-	      op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
-	    }
-	  op0 = memory_address_addr_space (mode, op0, as);
-	  mem = gen_rtx_MEM (mode, op0);
-	  set_mem_attributes (mem, tem, 0);
-	  set_mem_addr_space (mem, as);
-	  if (TREE_THIS_VOLATILE (tem))
-	    MEM_VOLATILE_P (mem) = 1;
+	  mem = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
 
 	  /* If the misaligned store doesn't overwrite all bits, perform
 	     rmw cycle on MEM.  */
@@ -4737,7 +4684,7 @@ expand_assignment (tree to, tree from, b
       else
 	{
 	  misalignp = false;
-	  to_rtx = expand_normal (tem);
+	  to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
 	}
 
       /* If the bitfield is volatile, we want to access it in the
@@ -9373,7 +9320,8 @@ expand_expr_real_1 (tree exp, rtx target
 	set_mem_attributes (temp, exp, 0);
 	set_mem_addr_space (temp, as);
 	align = get_object_or_type_alignment (exp);
-	if (mode != BLKmode
+	if (modifier != EXPAND_WRITE
+	    && mode != BLKmode
 	    && align < GET_MODE_ALIGNMENT (mode)
 	    /* If the target does not have special handling for unaligned
 	       loads of mode then it can use regular moves for them.  */
@@ -9461,7 +9409,8 @@ expand_expr_real_1 (tree exp, rtx target
 	set_mem_addr_space (temp, as);
 	if (TREE_THIS_VOLATILE (exp))
 	  MEM_VOLATILE_P (temp) = 1;
-	if (mode != BLKmode
+	if (modifier != EXPAND_WRITE
+	    && mode != BLKmode
 	    && align < GET_MODE_ALIGNMENT (mode)
 	    /* If the target does not have special handling for unaligned
 	       loads of mode then it can use regular moves for them.  */

Re: [PATCH 2/3] Move MEM_REF expansion to a new function

[Michael Matz]

Hi,

On Mon, 12 Mar 2012, Martin Jambor wrote:

> OK, the following patch changes the places where I previously called the 
> new function to call expand_expr with EXPAND_WRITE modifier and then 
> makes sure we do not perform reads into rvalues in expand_expr_real_1 in 
> the contexts where I need to avoid that.
> 
> So far it has passed bootstrap and testing on x86_64-linux, bootstraps 
> and testsuite runs on the other platforms are still underway.  What do 
> you think?

I like it, but can't approve.


Ciao,
Michael.

Re: [PATCH 2/3] Move MEM_REF expansion to a new function

[Richard Guenther]

On Tue, 13 Mar 2012, Michael Matz wrote:

> Hi,
> 
> On Mon, 12 Mar 2012, Martin Jambor wrote:
> 
> > OK, the following patch changes the places where I previously called the 
> > new function to call expand_expr with EXPAND_WRITE modifier and then 
> > makes sure we do not perform reads into rvalues in expand_expr_real_1 in 
> > the contexts where I need to avoid that.
> > 
> > So far it has passed bootstrap and testing on x86_64-linux, bootstraps 
> > and testsuite runs on the other platforms are still underway.  What do 
> > you think?
> 
> I like it, but can't approve.

Looks good to me, too.  Thus, ok for trunk if the other platform
tests succeed.

Thanks,
Richard.

Re: [PATCH 3/3] Misaligned MEM_REF reads

[Martin Jambor]

Hi,

On Mon, Mar 12, 2012 at 03:09:04PM +0100, Martin Jambor wrote:
> Hi,
> 
> this patch is very similar to the one I posted before
> (http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01377.html) except that
> it is now adjusted to sit on top of the new one before this and does
> not ignore complex numbers...

This is the same patch updated to apply on top of the new version of
the previous one (which I have committed a moment ago).  Again,
bootstrapped and tested on x86_64-linux (all languages including Ada
and Java), usparc64-linux (without Java) and ia64-linux (without
Ada), the i686 compile farm machine was not accessible yesterday so I
don't have any results from it but will start a test right now.

OK for trunk?

Thanks,

Martin



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

	* expr.c (expand_expr_real_1): handle misaligned scalar reads from
	memory through MEM_REFs by calling extract_bit_field.

	* testsuite/gcc.dg/misaligned-expand-1.c: New test.
	* testsuite/gcc.dg/misaligned-expand-3.c: Likewise.


Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -9411,21 +9411,27 @@ expand_expr_real_1 (tree exp, rtx target
 	  MEM_VOLATILE_P (temp) = 1;
 	if (modifier != EXPAND_WRITE
 	    && mode != BLKmode
-	    && align < GET_MODE_ALIGNMENT (mode)
-	    /* If the target does not have special handling for unaligned
-	       loads of mode then it can use regular moves for them.  */
-	    && ((icode = optab_handler (movmisalign_optab, mode))
-		!= CODE_FOR_nothing))
+	    && align < GET_MODE_ALIGNMENT (mode))
 	  {
-	    struct expand_operand ops[2];
+	    if ((icode = optab_handler (movmisalign_optab, mode))
+		!= CODE_FOR_nothing)
+	      {
+		struct expand_operand ops[2];
 
-	    /* We've already validated the memory, and we're creating a
-	       new pseudo destination.  The predicates really can't fail,
-	       nor can the generator.  */
-	    create_output_operand (&ops[0], NULL_RTX, mode);
-	    create_fixed_operand (&ops[1], temp);
-	    expand_insn (icode, 2, ops);
-	    return ops[0].value;
+		/* We've already validated the memory, and we're creating a
+		   new pseudo destination.  The predicates really can't fail,
+		   nor can the generator.  */
+		create_output_operand (&ops[0], NULL_RTX, mode);
+		create_fixed_operand (&ops[1], temp);
+		expand_insn (icode, 2, ops);
+		return ops[0].value;
+	      }
+	    else if (SLOW_UNALIGNED_ACCESS (mode, align))
+	      temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
+					0, TYPE_UNSIGNED (TREE_TYPE (exp)),
+					true, (modifier == EXPAND_STACK_PARM
+					       ? NULL_RTX : target),
+					mode, mode);
 	  }
 	return temp;
       }
Index: src/gcc/testsuite/gcc.dg/misaligned-expand-1.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/misaligned-expand-1.c
@@ -0,0 +1,41 @@
+/* Test that expand can generate correct loads of misaligned data even on
+   strict alignment platforms.  */
+
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+extern void abort ();
+
+typedef unsigned int myint __attribute__((aligned(1)));
+
+unsigned int
+foo (myint *p)
+{
+  return *p;
+}
+
+#define cst 0xdeadbeef
+#define NUM 8
+
+struct blah
+{
+  char c;
+  myint i[NUM];
+};
+
+struct blah g;
+
+int
+main (int argc, char **argv)
+{
+  int i, k;
+  for (k = 0; k < NUM; k++)
+    {
+      g.i[k] = cst;
+      i = foo (&g.i[k]);
+
+      if (i != cst)
+	abort ();
+    }
+  return 0;
+}
Index: src/gcc/testsuite/gcc.dg/misaligned-expand-3.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/misaligned-expand-3.c
@@ -0,0 +1,43 @@
+/* Test that expand can generate correct stores to misaligned data of complex
+   type even on strict alignment platforms.  */
+
+/* { dg-do run } */
+/* { dg-options "-O0" } */
+
+extern void abort ();
+
+typedef _Complex float mycmplx __attribute__((aligned(1)));
+
+void
+foo (mycmplx *p, float r, float i)
+{
+  __real__ *p = r;
+  __imag__ *p = i;
+}
+
+#define cvr 3.2f
+#define cvi 2.5f
+#define NUM 8
+
+struct blah
+{
+  char c;
+  mycmplx x[NUM];
+} __attribute__((packed));
+
+struct blah g;
+
+int
+main (int argc, char **argv)
+{
+  int k;
+
+  for (k = 0; k < NUM; k++)
+    {
+      foo (&g.x[k], cvr, cvi);
+      if (__real__ g.x[k] != cvr
+	  || __imag__ g.x[k] != cvi)
+	abort ();
+    }
+  return 0;
+}

Re: [PATCH 3/3] Misaligned MEM_REF reads

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5046 bytes --]

On Wed, 14 Mar 2012, Martin Jambor wrote:

> Hi,
> 
> On Mon, Mar 12, 2012 at 03:09:04PM +0100, Martin Jambor wrote:
> > Hi,
> > 
> > this patch is very similar to the one I posted before
> > (http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01377.html) except that
> > it is now adjusted to sit on top of the new one before this and does
> > not ignore complex numbers...
> 
> This is the same patch updated to apply on top of the new version of
> the previous one (which I have committed a moment ago).  Again,
> bootstrapped and tested on x86_64-linux (all languages including Ada
> and Java), usparc64-linux (without Java) and ia64-linux (without
> Ada), the i686 compile farm machine was not accessible yesterday so I
> don't have any results from it but will start a test right now.
> 
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 
> 2012-03-12  Martin Jambor  <mjambor@suse.cz>
> 
> 	* expr.c (expand_expr_real_1): handle misaligned scalar reads from
> 	memory through MEM_REFs by calling extract_bit_field.
> 
> 	* testsuite/gcc.dg/misaligned-expand-1.c: New test.
> 	* testsuite/gcc.dg/misaligned-expand-3.c: Likewise.
> 
> 
> Index: src/gcc/expr.c
> ===================================================================
> --- src.orig/gcc/expr.c
> +++ src/gcc/expr.c
> @@ -9411,21 +9411,27 @@ expand_expr_real_1 (tree exp, rtx target
>  	  MEM_VOLATILE_P (temp) = 1;
>  	if (modifier != EXPAND_WRITE
>  	    && mode != BLKmode
> -	    && align < GET_MODE_ALIGNMENT (mode)
> -	    /* If the target does not have special handling for unaligned
> -	       loads of mode then it can use regular moves for them.  */
> -	    && ((icode = optab_handler (movmisalign_optab, mode))
> -		!= CODE_FOR_nothing))
> +	    && align < GET_MODE_ALIGNMENT (mode))
>  	  {
> -	    struct expand_operand ops[2];
> +	    if ((icode = optab_handler (movmisalign_optab, mode))
> +		!= CODE_FOR_nothing)
> +	      {
> +		struct expand_operand ops[2];
>  
> -	    /* We've already validated the memory, and we're creating a
> -	       new pseudo destination.  The predicates really can't fail,
> -	       nor can the generator.  */
> -	    create_output_operand (&ops[0], NULL_RTX, mode);
> -	    create_fixed_operand (&ops[1], temp);
> -	    expand_insn (icode, 2, ops);
> -	    return ops[0].value;
> +		/* We've already validated the memory, and we're creating a
> +		   new pseudo destination.  The predicates really can't fail,
> +		   nor can the generator.  */
> +		create_output_operand (&ops[0], NULL_RTX, mode);
> +		create_fixed_operand (&ops[1], temp);
> +		expand_insn (icode, 2, ops);
> +		return ops[0].value;
> +	      }
> +	    else if (SLOW_UNALIGNED_ACCESS (mode, align))
> +	      temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
> +					0, TYPE_UNSIGNED (TREE_TYPE (exp)),
> +					true, (modifier == EXPAND_STACK_PARM
> +					       ? NULL_RTX : target),
> +					mode, mode);
>  	  }
>  	return temp;
>        }
> Index: src/gcc/testsuite/gcc.dg/misaligned-expand-1.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/misaligned-expand-1.c
> @@ -0,0 +1,41 @@
> +/* Test that expand can generate correct loads of misaligned data even on
> +   strict alignment platforms.  */
> +
> +/* { dg-do run } */
> +/* { dg-options "-O0" } */
> +
> +extern void abort ();
> +
> +typedef unsigned int myint __attribute__((aligned(1)));
> +
> +unsigned int
> +foo (myint *p)
> +{
> +  return *p;
> +}
> +
> +#define cst 0xdeadbeef
> +#define NUM 8
> +
> +struct blah
> +{
> +  char c;
> +  myint i[NUM];
> +};
> +
> +struct blah g;
> +
> +int
> +main (int argc, char **argv)
> +{
> +  int i, k;
> +  for (k = 0; k < NUM; k++)
> +    {
> +      g.i[k] = cst;
> +      i = foo (&g.i[k]);
> +
> +      if (i != cst)
> +	abort ();
> +    }
> +  return 0;
> +}
> Index: src/gcc/testsuite/gcc.dg/misaligned-expand-3.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/misaligned-expand-3.c
> @@ -0,0 +1,43 @@
> +/* Test that expand can generate correct stores to misaligned data of complex
> +   type even on strict alignment platforms.  */
> +
> +/* { dg-do run } */
> +/* { dg-options "-O0" } */
> +
> +extern void abort ();
> +
> +typedef _Complex float mycmplx __attribute__((aligned(1)));
> +
> +void
> +foo (mycmplx *p, float r, float i)
> +{
> +  __real__ *p = r;
> +  __imag__ *p = i;
> +}
> +
> +#define cvr 3.2f
> +#define cvi 2.5f
> +#define NUM 8
> +
> +struct blah
> +{
> +  char c;
> +  mycmplx x[NUM];
> +} __attribute__((packed));
> +
> +struct blah g;
> +
> +int
> +main (int argc, char **argv)
> +{
> +  int k;
> +
> +  for (k = 0; k < NUM; k++)
> +    {
> +      foo (&g.x[k], cvr, cvi);
> +      if (__real__ g.x[k] != cvr
> +	  || __imag__ g.x[k] != cvi)
> +	abort ();
> +    }
> +  return 0;
> +}
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer