diff mbox

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

Message ID 20120312140918.117967027@virgil.suse.cz
State New
Headers show

Commit Message

Martin Jambor March 12, 2012, 2:09 p.m. UTC 
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.

Comments

Richard Biener March 12, 2012, 2:34 p.m. UTC | #1 
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.
Michael Matz March 12, 2012, 3:26 p.m. UTC | #2 
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.
diff mbox

Patch

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))