* [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr
@ 2017-06-15 13:51 Eric Botcazou
2017-06-16 7:51 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Eric Botcazou @ 2017-06-15 13:51 UTC (permalink / raw)
To: gcc-patches
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr
2017-06-15 13:51 [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr Eric Botcazou
@ 2017-06-16 7:51 ` Richard Biener
2017-06-16 10:26 ` Eric Botcazou
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2017-06-16 7:51 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr
2017-06-16 7:51 ` Richard Biener
@ 2017-06-16 10:26 ` Eric Botcazou
2017-06-16 10:46 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Eric Botcazou @ 2017-06-16 10:26 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr
2017-06-16 10:26 ` Eric Botcazou
@ 2017-06-16 10:46 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2017-06-16 10:46 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-16 10:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-15 13:51 [patch] Do not directly change TARGET_MEM_REF in maybe_canonicalize_mem_ref_addr Eric Botcazou
2017-06-16 7:51 ` Richard Biener
2017-06-16 10:26 ` Eric Botcazou
2017-06-16 10:46 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).