[patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr

[patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Hi,

the transformation done to TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr 
is exactly the same as one of those done in maybe_fold_tmr, the latter is 
better written and the former function calls the latter, so this patch changes 
maybe_canonicalize_mem_ref_addr to avoid touching TARGET_MEM_REF directly.

Tested on x86-64/Linux, OK for the mainline?


2017-06-15  Eric Botcazou  <ebotcazou@adacore.com>

	PR bootstrap/80897
	* gimple-fold.c (maybe_canonicalize_mem_ref_addr): Do not change
	TARGET_MEM_REF expressions directly.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 558 bytes --]

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 249091)
+++ gimple-fold.c	(working copy)
@@ -4178,8 +4178,7 @@ maybe_canonicalize_mem_ref_addr (tree *t
 
   /* Canonicalize MEM [&foo.bar, 0] which appears after propagating
      of invariant addresses into a SSA name MEM_REF address.  */
-  if (TREE_CODE (*t) == MEM_REF
-      || TREE_CODE (*t) == TARGET_MEM_REF)
+  if (TREE_CODE (*t) == MEM_REF)
     {
       tree addr = TREE_OPERAND (*t, 0);
       if (TREE_CODE (addr) == ADDR_EXPR

Re: [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr

[Richard Biener]

On Thu, Jun 15, 2017 at 3:51 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> the transformation done to TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr
> is exactly the same as one of those done in maybe_fold_tmr, the latter is
> better written and the former function calls the latter, so this patch changes
> maybe_canonicalize_mem_ref_addr to avoid touching TARGET_MEM_REF directly.
>
> Tested on x86-64/Linux, OK for the mainline?

I don't think so.  get_address_description assumes TMR_BASE is in
canonical form,
that is, when it is an ADDR_EXPR we have a symbol and when not we have
a pointer.
TMR[&p->a] violates this and the gimple-fold.c part first canonicalizes this to
TMR[p + offsetof(a)].

Richard.

>
> 2017-06-15  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR bootstrap/80897
>         * gimple-fold.c (maybe_canonicalize_mem_ref_addr): Do not change
>         TARGET_MEM_REF expressions directly.
>
> --
> Eric Botcazou

Re: [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr

[Eric Botcazou]

> I don't think so.  get_address_description assumes TMR_BASE is in
> canonical form,
> that is, when it is an ADDR_EXPR we have a symbol and when not we have
> a pointer.
> TMR[&p->a] violates this and the gimple-fold.c part first canonicalizes this
> to TMR[p + offsetof(a)].

get_address_description doesn't assume anything on TMR_BASE:

void
get_address_description (tree op, struct mem_address *addr)
{
  if (TREE_CODE (TMR_BASE (op)) == ADDR_EXPR)
    {
      addr->symbol = TMR_BASE (op);
      addr->base = TMR_INDEX2 (op);
    }
  else
    {
      addr->symbol = NULL_TREE;
      if (TMR_INDEX2 (op))
	{
	  gcc_assert (integer_zerop (TMR_BASE (op)));
	  addr->base = TMR_INDEX2 (op);
	}
      else
	addr->base = TMR_BASE (op);
    }
  addr->index = TMR_INDEX (op);
  addr->step = TMR_STEP (op);
  addr->offset = TMR_OFFSET (op);
}

and maybe_fold_tmr will precisely turn TMR[&p->a] into TMR[p + offsetof(a)]:

  if (addr.symbol
      && TREE_CODE (TREE_OPERAND (addr.symbol, 0)) == MEM_REF)
    {
      addr.offset = fold_binary_to_constant
			(PLUS_EXPR, TREE_TYPE (addr.offset),
			 addr.offset,
			 TREE_OPERAND (TREE_OPERAND (addr.symbol, 0), 1));
      addr.symbol = TREE_OPERAND (TREE_OPERAND (addr.symbol, 0), 0);
      changed = true;
    }
  else if (addr.symbol
	   && handled_component_p (TREE_OPERAND (addr.symbol, 0)))
    {
      HOST_WIDE_INT offset;
      addr.symbol = build_fold_addr_expr
		      (get_addr_base_and_unit_offset
		         (TREE_OPERAND (addr.symbol, 0), &offset));
      addr.offset = int_const_binop (PLUS_EXPR,
				     addr.offset, size_int (offset));
      changed = true;
    }

The transformations are exactly the same in maybe_canonicalize_mem_ref_addr.

-- 
Eric Botcazou

Re: [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr

[Richard Biener]

On Fri, Jun 16, 2017 at 12:26 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I don't think so.  get_address_description assumes TMR_BASE is in
>> canonical form,
>> that is, when it is an ADDR_EXPR we have a symbol and when not we have
>> a pointer.
>> TMR[&p->a] violates this and the gimple-fold.c part first canonicalizes this
>> to TMR[p + offsetof(a)].
>
> get_address_description doesn't assume anything on TMR_BASE:
>
> void
> get_address_description (tree op, struct mem_address *addr)
> {
>   if (TREE_CODE (TMR_BASE (op)) == ADDR_EXPR)
>     {
>       addr->symbol = TMR_BASE (op);
>       addr->base = TMR_INDEX2 (op);
>     }
>   else
>     {
>       addr->symbol = NULL_TREE;
>       if (TMR_INDEX2 (op))
>         {
>           gcc_assert (integer_zerop (TMR_BASE (op)));
>           addr->base = TMR_INDEX2 (op);
>         }
>       else
>         addr->base = TMR_BASE (op);
>     }
>   addr->index = TMR_INDEX (op);
>   addr->step = TMR_STEP (op);
>   addr->offset = TMR_OFFSET (op);
> }
>
> and maybe_fold_tmr will precisely turn TMR[&p->a] into TMR[p + offsetof(a)]:
>
>   if (addr.symbol
>       && TREE_CODE (TREE_OPERAND (addr.symbol, 0)) == MEM_REF)
>     {
>       addr.offset = fold_binary_to_constant
>                         (PLUS_EXPR, TREE_TYPE (addr.offset),
>                          addr.offset,
>                          TREE_OPERAND (TREE_OPERAND (addr.symbol, 0), 1));
>       addr.symbol = TREE_OPERAND (TREE_OPERAND (addr.symbol, 0), 0);
>       changed = true;
>     }
>   else if (addr.symbol
>            && handled_component_p (TREE_OPERAND (addr.symbol, 0)))
>     {
>       HOST_WIDE_INT offset;
>       addr.symbol = build_fold_addr_expr
>                       (get_addr_base_and_unit_offset
>                          (TREE_OPERAND (addr.symbol, 0), &offset));
>       addr.offset = int_const_binop (PLUS_EXPR,
>                                      addr.offset, size_int (offset));
>       changed = true;
>     }
>
> The transformations are exactly the same in maybe_canonicalize_mem_ref_addr.

Well, it seems this just compensates for the fact
get_address_description is confused
and says it has a symbol when it has not.

I'd rather leave the canonicalization in a single place for both
MEM_REF and TARGET_MEM_REF
and instead remove the above code from maybe_fold_tmr (which is only called from
maybe_canonicalize_mem_ref_addr btw.  Inlining it (and thus exporting
create_mem_ref_raw)
would work for me as well and likely reduce the confusion as to what
is done where.

Richard.

> --
> Eric Botcazou