Pointer Plus Patch

[Andrew Pinski]

On 6/13/07, Paolo Bonzini <bonzini@gnu.org> wrote:

> > + /* The variable a cannot be a local variable as we get better aliasing
> > +    now and decide that the store to a is dead.  The better aliasing comes
> > +    from better representation of pointer arthimetic. */
> > + long a = 10;
>
> Just say "This variable is global to avoid that stores to it be
> optimized away".
>
> Actually, I don't understand this change.  It seems to me that the
> compiler is wrong in optimizing away the store to a, unless this is because
>
>    f((long)(&a)-1,0);
>
> creates a pointer outside the object "a"?  If so, you should modify the
> test to use (long)(&a)+1 and correspondingly *((long *)(a-1).

Hmm, I don't care which way this goes, I was just fixing the testcase
:).  The testcase I think can't happen anyways because we no longer
cannot split live ranges so expand will never get the same IR as
before.


>
> > !   /* Vectorizable, before pointer plus we would get a redundant cast
> > !      (caused by ponter arithmetics), alias analysis fails to distinguish
> > !      between the pointers.  */
>
> Who cares.  /* Vectorizable.  */ and that's it.

I wanted to explain why this was changed.

>
> > !       /* Since the middle-end checks the type when doing a build2, we
> > !      need to build the tree in pieces.
>
> Not so disgusting, but it would be nicer if you built a
> non-type-checking code (e.g. MIN_EXPR) and replaced the code, instead of
> replacing operand 0.

Actually tree codes could have difference sizes (or maybe in the
future they can) which is why I did it this way.


> > !       offset = fold_convert (sizetype,
> > !                          int_const_binop (PLUS_EXPR, offset, offset2, 1));
>
> size_binop?  Actually, since you are at it you could try to simplify the
> code because the last parameter to maybe_fold_stmt_indirect (i.e.
> offset) is always integer_zero_node.

Actually int_const_binop is faster and size_binop could cause an ICE.

> > +     case POINTER_PLUS_EXPR:
> > +       /* Even though the sizetype mode and the pointer's mode can be different
> > +          expand is able to handle this correct and get the correct result out
> > +          from the PLUS_EXPR code.  */
>
> The comment is useless, the code seems pretty obvious even without it
> (it's not to you because you know the history of the code very well).

Actually it is not that useless as there really cases where this can
happen (try using mode with pointers on s390x) and it is not that
obviously what happens there.

>
> > !           = convert (sizetype,
> >                        size_diffop (size_zero_node, gnu_pos));
>
> Use fold_convert instead.

This was already using convert.  I did not change this

>
> >             gnu_ptr = convert (gnu_char_ptr_type, gnu_ptr);
>
> Well, I'd use it here too but that's your call.

Actually it is the Ada maintainers call.  I did not want to change the
front-ends too much for the branch.

>
> > !       return build_binary_op (POINTER_PLUS_EXPR, type, expr,
> > !                               fold (convert (sizetype, byte_diff)));
> >       }
>
> Just use fold_convert here too, as byte_diff is returned from
> size_diffop here too.

Again I did not want to change the front-end too much from what it was before.

>
> > +       /* FALL THROUGH */
>
> lowercase.

There is no clear style recorded for this case and GCC sources are
with a couple of different styles with reguard to this.  I am using
the one word all caps one.

>
> >           inc = c_size_in_bytes (TREE_TYPE (result_type));
> > +         inc = convert (sizetype, inc);
>
> Return value of c_size_in_bytes should already be a sizetype.  Use
> fold_convert if this is not the case.
>
> > !         inc = integer_one_node;
> > !         inc = convert (argtype, inc);
>
> I wonder if we should use build_int_cst here too, since convert could
> cause overflow.  Testcase:
>
>    struct x {
>      signed x:1;
>    };
>
>    void f(struct x *y)
>    {
>      y->x++;
>    }

The code before was already using convert, I did not change this at
all.  I just pushed the convert into the two sides of the if.

>
> > !           && TREE_ADDRESSABLE (decl)
> > !                 /* XXX: not sufficient: return-by-value structs! */
>
> Irrelevant change, or move the comment together with the previous one.
> There are other whitespace changes in tree-mudflap.c.

Yes I changed that on accident while fixing mudflap.  I will commit to
the trunk the white space changes before I do the merge.

>
> >           ofs = convert (bitsizetype, TREE_OPERAND (t, 2));
>
> Please turn this into fold_convert since you are at it.
>
> >           addr = convert (ptr_type_node, addr);
>
> Likewise.

You ask me to remove whitespace changes but ask me to change unrelated
converts to fold_convert.  I think those should be done seperately.

>
> >     /* If the use of the ADDR_EXPR must be a PLUS_EXPR, or else there
> >        is nothing to do. */
> > !   if (TREE_CODE (rhs) != POINTER_PLUS_EXPR)
> >       return false;
>
> If you can, please fix the English in this comment.
I will change this because I need to change it to be POINTER_PLUS_EXPR anyways:
  /* If the use of the ADDR_EXPR is not a POINTER_PLUS_EXPR, there
     is nothing to do. */



Thanks,
Andrew Pinski