[RFC] Remove call to build_ref_for_offset in IPA-SRA

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

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?

> -         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)

> +               {
> +                 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.

> +               }
>            }
> -         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?

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.

Thanks,
Richard.

> +         /* !!! Remove after testing */
> +         if (dump_file)
>            {
> -             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);
> +             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
> @@ -8709,8 +8709,11 @@ expand_expr_real_1 (tree exp, rtx target
>                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)))))
> +                   && ((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
> @@ -11,7 +11,7 @@ extern "C" {
>     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 "" } */
> +           __warn_memset_zero_len ();
>     }
>  }
>  inline void clear_mem(void* ptr, u32bit n)    {
>