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

[Richard Biener <richard.guenther@gmail.com>]

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