| Message ID | 59903573.zuAO1vMF2R@polaris |
|---|---|
| State | New |
| Headers | show |
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
> 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.
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
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