[RFC PATCH] Remove call to build_ref_for_offset in IPA-SRA
Martin Jambor
mjambor@suse.cz
Thu Aug 5 13:18:00 GMT 2010
Hi,
On Tue, Aug 03, 2010 at 12:03:03PM +0200, Richard Guenther wrote:
> On Mon, Aug 2, 2010 at 10:20 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > Hi,
> >
> > the following is another patch in the series that eventually should
> > re-implement build_ref_for_offset. Â It basically re-implements it in
> > the IPA-SRA user of the function, I might eventually decide to use the
> > new implementation of the function instead but I fairly like it
> > separate because this (current) user of the function 1) always gets
> > offsets aligned to bytes and 2) it can be fed pointers, unlike all
> > others.
> >
> > Anyway, the problem with this patch is of course the hunk in expr.c.
> > However, without it I get ICEs when compiling testcase
> > g++.dg/torture/pr36445.C on x86_64-linux even though I believe I
> > produce correct gimple. Â As far as I understand the code, if the V_C_E
> > path is not taken and we try to build and expand a BIT_FIELD_REF, I
> > get something that is not BLKmode because its argument is not and this
> > is not expected by emit_block_move which attempts to use it. Â I am not
> > sure what exactly and exactly under what circumstances needs to behave
> > differently. Â I intend to go back to this problem but I am new to the
> > code, and thus I would appreciate any help with that. Â Otherwise the
> > patch bootstraps and tests fine.
> >
> > Thanks,
> >
> > Martin
> >
> >
> > 2010-07-28 Martin Jambor <mjambor@suse.cz>
> >
> > * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
> > calling build_ref_for_offset.
> > * expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
> > when sizes don't match but expected mode is BLKmode.
> >
> > * testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
> >
> > Index: mine/gcc/ipa-prop.c
> > ===================================================================
> > --- mine.orig/gcc/ipa-prop.c
> > +++ mine/gcc/ipa-prop.c
> > @@ -2149,40 +2149,63 @@ ipa_modify_call_arguments (struct cgraph
> > }
> > else if (!adj->remove_param)
> > {
> > - tree expr, orig_expr;
> > - bool allow_ptr, repl_found;
> > + tree expr, base, off;
> > + location_t loc;
> >
> > - orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
> > - if (TREE_CODE (expr) == ADDR_EXPR)
> > - {
> > - allow_ptr = false;
> > - expr = TREE_OPERAND (expr, 0);
> > - }
> > - else
> > - allow_ptr = true;
> > + gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
> > + base = gimple_call_arg (stmt, adj->base_index);
> > + loc = EXPR_LOCATION (base);
>
> If I read the following code correctly we build an access to
> &base + offset here? The old allow_ptr code and the
> replacement looks odd. Can't we just store a 'deref_p' flag
> in the ipa-parm adjustment structure?
>
> Please add a comment what we are doing here.
>
> What are we actually replacing? A ptr argument that is
> dereferenced by a scalar arg? An aggregate by-value arg
> with separate by-value args?
Note that this is the part where we transform calls in callees of the
function we are actually transforming. I'm adding the following
comment:
/* We create a new parameter out of the value of the old one, we can
do the following kind of transformations:
- A scalar passed by reference is converted to a scalar passed by
value. (adj->by_ref is false and the type of the original
actual argument is a pointer to a scalar).
- A part of an aggregate is passed instead of the whole aggregate.
The part can be passed either by value or by reference, this is
determined by value of adj->by_ref. Moreover, the code below
handles both situations when the original aggregate is passed by
value (its type is not a pointer) and when it is passed by
reference (it is a pointer to an aggregate).
When the new argument is passed by reference (adj->by_ref is true)
it must be a part of an aggregate and therefore we form it by
simply taking the address of a reference inside the original
aggregate. */
Turning scalars for which we have a decl in this function and which
are passed by reference into arguments by value is currently really
done by folding the resulting MEM_REF. Perhaps I should special-case
it here. But sometimes we're just dereferencing a pointer in the
caller and don't have a decl.
>
> > - repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
> > - adj->offset, adj->type,
> > - allow_ptr);
> > - if (repl_found)
> > + if ((TREE_CODE (base) == ADDR_EXPR
> > + && DECL_P (TREE_OPERAND (base, 0)))
> > + || (TREE_CODE (base) != ADDR_EXPR
> > + && POINTER_TYPE_P (TREE_TYPE (base))))
> > + off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
> > + else
> > {
> > - if (adj->by_ref)
> > - expr = build_fold_addr_expr (expr);
> > + HOST_WIDE_INT base_offset;
> > + tree prev_base;
> > +
> > + if (TREE_CODE (base) == ADDR_EXPR)
> > + base = TREE_OPERAND (base, 0);
> > + prev_base = base;
> > + base = get_addr_base_and_unit_offset (base, &base_offset);
> > + if (!base)
>
> Ugh - when does that happen? (it shouldn't)
Any time we bump into an array_ref with a non-constant index (such as
in ptr->array[i].structure). I believe we actually discussed this on
IRC. What happens next is that I build a MEM_REF of and ADDR_EXPR of
a handled_component (which may possibly be of a MEM_REF) which is
bogus on its own but it is handled well by the subsequent call to
force_gimple_operand (I hadle it myself in the actual reimplementation
of build_ref_for_offsets because there I would not call
force_gimple_operand anyway).
>
> > + {
> > + base = build_fold_addr_expr (prev_base);
> > + off = build_int_cst (TREE_TYPE (base),
> > + adj->offset / BITS_PER_UNIT);
> > + }
> > + else if (TREE_CODE (base) == MEM_REF)
> > + {
> > + off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
> > + base_offset
> > + + adj->offset / BITS_PER_UNIT);
> > + off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
> > + off, 0);
> > + base = TREE_OPERAND (base, 0);
> > + }
> > + else
> > + {
> > + off = build_int_cst (build_pointer_type (TREE_TYPE (base)),
> > + base_offset
> > + + adj->offset / BITS_PER_UNIT);
> > + base = build_fold_addr_expr (base);
>
> You are choosing some random types for the offset. Please try
> to preserve that of the original access. (It's not clear to me what
> we are replacing with what here ...)
>
> See tree.c:reference_alias_ptr_type for a way to get the alias
> type of an existing reference.
Well, I don't really understand what you mean by random here. It is
true that I don't go for the TYPE_MAIN_VARIANT, is that the problem?
I have changed the patch a bit to actually use
reference_alias_ptr_type. If the original argument came from a
MEM_REF, I do preserve the type from it. If we have a base decl, I
use a pointer to it (or now its main variant). Only when I just have
a pointer I simply trust its type. Is that wrong now, do I somehow
have to propagate the reference type from the callee to the caller
here?
>
> > + }
> > }
> > - else
> > +
> > + expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
> > + if (adj->by_ref)
> > + expr = build_fold_addr_expr (expr);
>
> Separating the by_ref (or allow_ptr?!) cases could make things
> easier to understand and avoid stripping off and re-building ADDR_EXPRs
> all the time?
With this patch there is no allow_ptr any more, all the other callers
pass false for this parameter to build_ref_for_offset - moreover,
allow_ptr simply allowed the original parameter - the one which is
being transformed into a new one - to be passed by reference (and it
was more of a sanity check anyway. On the contrary, when adj->by_ref
is true, it means that the newly created parameter should be passed by
reference. We make it such by simply encapsulating the reference into
an aggregate in an ADDR_EXPR. Building of the reference would be
exactly the same and so I don't think separating the case would help
to streamline this.
>
> I have a hard time extracting patches from google-mail, so if you
> can bounce me the patch to my suse address I can have a look
> at the expr.c hunk.
I believe that in gmail, in the menu next to the reply button, there
is a "Show original" command that can help with extracting patches.
Anyway, thanks for looking into this. Below is a context diff of the
slightly amended version which might be more readable and I will also
attach unified diff. I'll also CC your suse address.
Martin
2010-08-04 Martin Jambor <mjambor@suse.cz>
* ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
calling build_ref_for_offset.
* expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
when sizes don't match but expected mode is BLKmode.
* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
Index: mine/gcc/ipa-prop.c
===================================================================
*** mine.orig/gcc/ipa-prop.c
--- mine/gcc/ipa-prop.c
*************** ipa_modify_call_arguments (struct cgraph
*** 2149,2188 ****
}
else if (!adj->remove_param)
{
! tree expr, orig_expr;
! bool allow_ptr, repl_found;
! orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
! if (TREE_CODE (expr) == ADDR_EXPR)
! {
! allow_ptr = false;
! expr = TREE_OPERAND (expr, 0);
! }
! else
! allow_ptr = true;
! repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
! adj->offset, adj->type,
! allow_ptr);
! if (repl_found)
{
! if (adj->by_ref)
! expr = build_fold_addr_expr (expr);
}
! else
{
! tree ptrtype = build_pointer_type (adj->type);
! expr = orig_expr;
! if (!POINTER_TYPE_P (TREE_TYPE (expr)))
! expr = build_fold_addr_expr (expr);
! if (!useless_type_conversion_p (ptrtype, TREE_TYPE (expr)))
! expr = fold_convert (ptrtype, expr);
! expr = fold_build2 (POINTER_PLUS_EXPR, ptrtype, expr,
! build_int_cst (sizetype,
! adj->offset / BITS_PER_UNIT));
! if (!adj->by_ref)
! expr = fold_build1 (INDIRECT_REF, adj->type, expr);
}
expr = force_gimple_operand_gsi (&gsi, expr,
adj->by_ref
|| is_gimple_reg_type (adj->type),
--- 2149,2232 ----
}
else if (!adj->remove_param)
{
! tree expr, base, off;
! location_t loc;
! /* We create a new parameter out of the value of the old one, we can
! do the following kind of transformations:
!
! - A scalar passed by reference is converted to a scalar passed by
! value. (adj->by_ref is false and the type of the original
! actual argument is a pointer to a scalar).
!
! - A part of an aggregate is passed instead of the whole aggregate.
! The part can be passed either by value or by reference, this is
! determined by value of adj->by_ref. Moreover, the code below
! handles both situations when the original aggregate is passed by
! value (its type is not a pointer) and when it is passed by
! reference (it is a pointer to an aggregate).
!
! When the new argument is passed by reference (adj->by_ref is true)
! it must be a part of an aggregate and therefore we form it by
! simply taking the address of a reference inside the original
! aggregate. */
!
! gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
! base = gimple_call_arg (stmt, adj->base_index);
! loc = EXPR_LOCATION (base);
! if (TREE_CODE (base) == ADDR_EXPR
! && DECL_P (TREE_OPERAND (base, 0)))
! off = build_int_cst (reference_alias_ptr_type (base),
! adj->offset / BITS_PER_UNIT);
! else if (TREE_CODE (base) != ADDR_EXPR
! && POINTER_TYPE_P (TREE_TYPE (base)))
! off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
! else
{
! HOST_WIDE_INT base_offset;
! tree prev_base;
!
! if (TREE_CODE (base) == ADDR_EXPR)
! base = TREE_OPERAND (base, 0);
! prev_base = base;
! base = get_addr_base_and_unit_offset (base, &base_offset);
! if (!base)
! {
! base = build_fold_addr_expr (prev_base);
! off = build_int_cst (reference_alias_ptr_type (prev_base),
! adj->offset / BITS_PER_UNIT);
! }
! else if (TREE_CODE (base) == MEM_REF)
! {
! off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
! base_offset
! + adj->offset / BITS_PER_UNIT);
! off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
! off, 0);
! base = TREE_OPERAND (base, 0);
! }
! else
! {
! off = build_int_cst (reference_alias_ptr_type (base),
! base_offset
! + adj->offset / BITS_PER_UNIT);
! base = build_fold_addr_expr (base);
! }
}
!
! expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
! if (adj->by_ref)
! expr = build_fold_addr_expr (expr);
!
! /* !!! Remove after testing */
! if (dump_file)
{
! fprintf (dump_file, "Built MEM_REF: ");
! print_generic_expr (dump_file, expr, 0);
! fprintf (dump_file, "\n");
}
+
expr = force_gimple_operand_gsi (&gsi, expr,
adj->by_ref
|| is_gimple_reg_type (adj->type),
Index: mine/gcc/expr.c
===================================================================
*** mine.orig/gcc/expr.c
--- mine/gcc/expr.c
*************** expand_expr_real_1 (tree exp, rtx target
*** 8709,8716 ****
tree bftype;
if (offset == 0
&& host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
! && (GET_MODE_BITSIZE (DECL_MODE (base))
! == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))
return expand_expr (build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (exp), base),
target, tmode, modifier);
--- 8709,8719 ----
tree bftype;
if (offset == 0
&& host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
! && ((GET_MODE_BITSIZE (DECL_MODE (base))
! == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))
! || (TYPE_MODE (TREE_TYPE (exp)) == BLKmode
! && (GET_MODE_BITSIZE (DECL_MODE (base))
! >= TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))))
return expand_expr (build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (exp), base),
target, tmode, modifier);
Index: mine/gcc/testsuite/g++.dg/torture/pr34850.C
===================================================================
*** mine.orig/gcc/testsuite/g++.dg/torture/pr34850.C
--- mine/gcc/testsuite/g++.dg/torture/pr34850.C
*************** extern "C" {
*** 11,17 ****
extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
void * memset (void *__dest, int __ch, size_t __len) throw () {
if (__builtin_constant_p (__len) && __len == 0)
! __warn_memset_zero_len (); /* { dg-warning "" } */
}
}
inline void clear_mem(void* ptr, u32bit n) {
--- 11,17 ----
extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
void * memset (void *__dest, int __ch, size_t __len) throw () {
if (__builtin_constant_p (__len) && __len == 0)
! __warn_memset_zero_len ();
}
}
inline void clear_mem(void* ptr, u32bit n) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: no_bro_in_ipaprop.diff
Type: text/x-patch
Size: 5918 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20100805/3a1df22e/attachment.bin>
More information about the Gcc-patches
mailing list