Re: Regression due to tree loop opt / expand weirdness

Re: Regression due to tree loop opt / expand weirdness

[Richard Kenner]

    As I don't have the code in question, could you elaborate why
    you think the current test in expand_expr_addr_expr would not
    work for ia64/VMS?  What's the difference to s390x/TPF?

I haven't followed that part of the discussion.  I was merely seconding
RTH's comment that these types are needed in such systems.

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Kenner wrote:
> 
>     As I don't have the code in question, could you elaborate why
>     you think the current test in expand_expr_addr_expr would not
>     work for ia64/VMS?  What's the difference to s390x/TPF?
> 
> I haven't followed that part of the discussion.  I was merely seconding
> RTH's comment that these types are needed in such systems.

OK, I certainly agree with *that*; we need those types on TPF.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Kenner wrote:
> 
>     I could not find any such ia64 system in current GCC CVS, are you talking
>     about an out-of-tree port?
> 
> Correct. It's still being developed.

I see, that's the piece I was missing.

As I don't have the code in question, could you elaborate why
you think the current test in expand_expr_addr_expr would not
work for ia64/VMS?  What's the difference to s390x/TPF?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Richard Kenner]

    I could not find any such ia64 system in current GCC CVS, are you talking
    about an out-of-tree port?

Correct. It's still being developed.

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Kenner wrote:
> 
>     First of all, the above change is provably a no-op on ia64 since
>     the valid_pointer_mode implementation on ia64 is the default one,
>     which is exactly (mode != Pmode && mode != ptr_mode).
> 
> Please read what I wrote.  On some ia64 systems (e.g., VMS), Pmode and
> ptr_mode are both DImode but you need to support 32-bit pointers.  On
> those ia64 systems, target.valid_pointer_mode is overridden to allow both
> SImode and DImode.

I could not find any such ia64 system in current GCC CVS, are you talking
about an out-of-tree port?

In any case, as I said, s390x/TPF is in exactly this situation (Pmode ==
ptr_mode == DImode, valid_pointer_mode overridden to allow both SImode
and DImode); and this case is exactly the one that my patch fixes.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Richard Kenner]

    First of all, the above change is provably a no-op on ia64 since
    the valid_pointer_mode implementation on ia64 is the default one,
    which is exactly (mode != Pmode && mode != ptr_mode).

Please read what I wrote.  On some ia64 systems (e.g., VMS), Pmode and
ptr_mode are both DImode but you need to support 32-bit pointers.  On
those ia64 systems, target.valid_pointer_mode is overridden to allow both
SImode and DImode.

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Kenner wrote:
>     > I can't imagine except that would break things when you *do*
>     > want to use "typedef void *ptr32 __attribute__((mode(SI)));".
> 
>     What do you think would break?  I assume you're talking about
>     s390x, not ia64 (as on ia64 there wouldn't be any difference).
> 
> On ia64/VMS, the default pointer size is 64 bits.  But you have to
> pass 32-bit addresses to some system calls.  Hence the need to support
> the above typedef.

This is exactly the same situation as on s390x/TPF, and this is 
exactly the case which was broken without my patch and works
*with* my patch.  (B.t.w. right now ia64/VMS does *not* allow
the above typedef as it uses the default valid_pointer_mode.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Kenner wrote:
>     Would changing the test in expand_expr_addr_expr from 
>     
>       if (!target.valid_pointer_mode (tmode))
> 
>     to
> 
>       if (tmode != Pmode && tmode != ptr_mode)
> 
>     be OK with you?  This should cause no change to ia64 ...
> 
> That looks wrong to me.  On ia64, Pmode is always DImode, but ptr_mode can
> be either SImode or DImode depending on the *default* pointer size for
> the configuration.  But we want to support both 32-bit and 64-bit pointers
> for IA64 on at least some configurations (e.g., VMS) no matter what the
> default is.
> 
> The target.valid_pointer_mode test does exactly the right thing and seems
> to me to be the only thing that does.

First of all, the above change is provably a no-op on ia64 since
the valid_pointer_mode implementation on ia64 is the default one,
which is exactly (mode != Pmode && mode != ptr_mode).

In addition, if you look further down in expand_expr_addr_expr,
tmode is passed as argument to convert_memory_address, and this
routine by design allows only Pmode and ptr_mode as target mode.
If we pass in anything else, we get ICEs -- which is exactly 
the problem I had.  So if we indeed need to allow more modes at
some point, this will require much more extensive changes than
just this if condition.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Richard Kenner]

    > I can't imagine except that would break things when you *do*
    > want to use "typedef void *ptr32 __attribute__((mode(SI)));".

    What do you think would break?  I assume you're talking about
    s390x, not ia64 (as on ia64 there wouldn't be any difference).

On ia64/VMS, the default pointer size is 64 bits.  But you have to
pass 32-bit addresses to some system calls.  Hence the need to support
the above typedef.

Re: Regression due to tree loop opt / expand weirdness

[Richard Kenner]

    Would changing the test in expand_expr_addr_expr from 
    
      if (!target.valid_pointer_mode (tmode))

    to

      if (tmode != Pmode && tmode != ptr_mode)

    be OK with you?  This should cause no change to ia64 ...

That looks wrong to me.  On ia64, Pmode is always DImode, but ptr_mode can
be either SImode or DImode depending on the *default* pointer size for
the configuration.  But we want to support both 32-bit and 64-bit pointers
for IA64 on at least some configurations (e.g., VMS) no matter what the
default is.

The target.valid_pointer_mode test does exactly the right thing and seems
to me to be the only thing that does.

Re: Regression due to tree loop opt / expand weirdness

[Zdenek Dvorak]

Hello,

> >expander seems to expect that pointer is first casted to integer
> >of the same size, and then to the target type (at least this is the way
> >such a cast is translated by C frontend).  This intermediate cast seems
> >superfluous to me (and to folder, apparently); fixing expand so that
> >it does not have such a weird restriction is IMHO the right way
> >(although it of course would not be hard to persuade fold_convert to
> >produce the extra cast).
> 
> Surely makes sense, but this is not what your patch to expand_binop did.

it was one step in that direction.  There apparently are other problems
in expand not covered by that patch.

Zdenek

Re: Regression due to tree loop opt / expand weirdness

[Paolo Bonzini]

> expander seems to expect that pointer is first casted to integer
> of the same size, and then to the target type (at least this is the way
> such a cast is translated by C frontend).  This intermediate cast seems
> superfluous to me (and to folder, apparently); fixing expand so that
> it does not have such a weird restriction is IMHO the right way
> (although it of course would not be hard to persuade fold_convert to
> produce the extra cast).

Surely makes sense, but this is not what your patch to expand_binop did.

Paolo

Re: Regression due to tree loop opt / expand weirdness

[Zdenek Dvorak]

Hello,

> >could you please explain in more detail what you mean by "wrong"?  It
> >preserves semantics of the program (unless there is some weird property
> >of pointers I am not aware of).
> 
> I'll try to answer since your timezone is more similar to mine than to 
> Richard's.
> 
> Since expr.c seemed to dislike doing operations on pointers in modes 
> different from Pmode, many people said in the other thread that your 
> optimization is not respecting some invariant that the expander expects.

expander seems to expect that pointer is first casted to integer
of the same size, and then to the target type (at least this is the way
such a cast is translated by C frontend).  This intermediate cast seems
superfluous to me (and to folder, apparently); fixing expand so that
it does not have such a weird restriction is IMHO the right way
(although it of course would not be hard to persuade fold_convert to
produce the extra cast).

Zdenek

Re: Regression due to tree loop opt / expand weirdness

[Paolo Bonzini]

> could you please explain in more detail what you mean by "wrong"?  It
> preserves semantics of the program (unless there is some weird property
> of pointers I am not aware of).

I'll try to answer since your timezone is more similar to mine than to 
Richard's.

Since expr.c seemed to dislike doing operations on pointers in modes 
different from Pmode, many people said in the other thread that your 
optimization is not respecting some invariant that the expander expects.

Paolo

Re: Regression due to tree loop opt / expand weirdness

[Zdenek Dvorak]

Hello,

> On Sun, Oct 03, 2004 at 09:40:12PM +0200, Ulrich Weigand wrote:
> > Richard, I appears you've added the valid_pointer_mode check to 
> > expand_expr_addr_expr -- could you elaborate how the back-end is
> > supposed to handle this?
> 
> The reason to allow non-Pmode is that otherwise ilp32 on 64-bit targets
> breaks.  That's the narrowest condition I could find that keeps ia64-hpux
> working.  Computing symbol_ref in ptr_mode and not Pmode is not a new
> thing, it's been done for quite some time.
> 
> As for why the tree loop optimizer is playing such silly narrowing games
> with pointers, ask Zdenek.  I've already expressed my opinion that it's
> wrong, but it has been an uphill fight trying to convince him.

could you please explain in more detail what you mean by "wrong"?  It
preserves semantics of the program (unless there is some weird property
of pointers I am not aware of).

It may be true that in some cases using this possibility may not be the
best choice from the performance point of view (I by no means claim that
the cost function in ivopts is perfect; once I get over the worse
problems with new loop optimizer, I will try to improve it).  However
in general I believe it is perfectly legal, and in some (not so weird)
cases also the best possibility.

Zdenek

Re: Regression due to tree loop opt / expand weirdness

[Eric Christopher]

> > I stand by my assertion that it's a tree-level loop bug that we are
> > asking for these narrowed pointers in the first place.
> 
> That's what I'd have thought as well ...

Perhaps a comment in the code to that effect?

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Henderson wrote:

> I suspect that you'll have problems somewhere, though I can't point
> my finger at exactly where.  If you want to wait and find it later,
> then I guess I don't care since it doesn't affect me or mine; go
> ahead with your patch.

I've committed the patch now, since it is certainly much better
than the current situation.  If there will be problems, we can
deal with them when and if they occur.

> I stand by my assertion that it's a tree-level loop bug that we are
> asking for these narrowed pointers in the first place.

That's what I'd have thought as well ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Richard Henderson]

On Mon, Oct 04, 2004 at 12:21:19AM +0200, Ulrich Weigand wrote:
> If you still don't like it, I'd appreciate any suggestions how to
> fix the current problems ...

I suspect that you'll have problems somewhere, though I can't point
my finger at exactly where.  If you want to wait and find it later,
then I guess I don't care since it doesn't affect me or mine; go
ahead with your patch.

I stand by my assertion that it's a tree-level loop bug that we are
asking for these narrowed pointers in the first place.  Until that's
fixed, we will continue to encounter things that appear wrong in
expand_expr_addr_expr.



r~

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Henderson wrote:

> I can't imagine except that would break things when you *do*
> want to use "typedef void *ptr32 __attribute__((mode(SI)));".

What do you think would break?  I assume you're talking about
s390x, not ia64 (as on ia64 there wouldn't be any difference).

The intended use of SImode pointers on s390x is *not* to hold
machine addresses in the current addressing mode.  Instead,
it is intended to simplify writing code that interfaces to
code running in another addressing mode (think something like
the 32-bit system call compatibility layer in the Linux kernel).
[ This tends to occur more frequently in TPF than in Linux,
which is why the TPF folks requested that feature.  ]

You want to declare variables and data structures holding 31-bit
pointers, so that you can easily access the data pointed to 
from within GCC-generated 64-bit code.  

So writing 

  typedef int *ptr32 __attribute__ ((mode (SI)));
  
  ptr32 p;
  use (*p);

should generate code equivalent to

  typedef int ptr32;

  ptr32 p;
  use (*(int *)(size_t)(p & 0x7fffffff));

except it is easier to write and provides additional type checks.


It is not really required that 64-bit code like

  ptr32 p;
  int i;

  p = &i;

does something sensible (it ususally cannot as &i isn't representible
in 32-bit) or is implemented efficiently.  However, even *if* you
want this to work by storing the truncated pointer value, it does
work just fine with my patch.  In fact, *without* my patch this
test case ICEs currently.
 

So the way things used to work (all address arithmetic performed in
DImode) was just fine for the intended use of mode((SI)) pointers
on s390x.

Or, to put it another way, IMO valid_pointer_mode should be a pure
front-end issue, providing syntactic sugar to simplify writing cross-
mode code, but should not change anything w.r.t. code generation
or the back end.  (B.t.w. it appears valid_pointer_mode isn't
actually documented anywhere :-/)

In the mean time bootstrap and regression tests have finished 
succesfully for the patch below on s390-ibm-linux and s390x-ibm-linux.

If you still don't like it, I'd appreciate any suggestions how to
fix the current problems ...

Thanks,
Ulrich



ChangeLog:

	* expr.c (expand_expr_addr_expr): Only accept Pmode or ptr_mode
	as valid modes to expand address expressions.

Index: gcc/expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.725
diff -c -p -r1.725 expr.c
*** gcc/expr.c	28 Sep 2004 22:54:57 -0000	1.725
--- gcc/expr.c	3 Oct 2004 20:16:29 -0000
*************** expand_expr_addr_expr (tree exp, rtx tar
*** 6201,6207 ****
    /* We can get called with some Weird Things if the user does silliness
       like "(short) &a".  In that case, convert_memory_address won't do
       the right thing, so ignore the given target mode.  */
!   if (!targetm.valid_pointer_mode (tmode))
      tmode = Pmode;
  
    result = expand_expr_addr_expr_1 (TREE_OPERAND (exp, 0), target,
--- 6201,6207 ----
    /* We can get called with some Weird Things if the user does silliness
       like "(short) &a".  In that case, convert_memory_address won't do
       the right thing, so ignore the given target mode.  */
!   if (tmode != Pmode && tmode != ptr_mode)
      tmode = Pmode;
  
    result = expand_expr_addr_expr_1 (TREE_OPERAND (exp, 0), target,


-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Richard Henderson]

On Sun, Oct 03, 2004 at 10:05:37PM +0200, Ulrich Weigand wrote:
> Would changing the test in expand_expr_addr_expr from 
> 
>   if (!target.valid_pointer_mode (tmode))
> 
> to
> 
>   if (tmode != Pmode && tmode != ptr_mode)
> 
> be OK with you?  This should cause no change to ia64 ...

I can't imagine except that would break things when you *do*
want to use "typedef void *ptr32 __attribute__((mode(SI)));".

So, no, this isn't ok.


r~

Re: Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Richard Henderson wrote:
> On Sun, Oct 03, 2004 at 09:40:12PM +0200, Ulrich Weigand wrote:
> > Richard, I appears you've added the valid_pointer_mode check to 
> > expand_expr_addr_expr -- could you elaborate how the back-end is
> > supposed to handle this?
> 
> The reason to allow non-Pmode is that otherwise ilp32 on 64-bit targets
> breaks.  That's the narrowest condition I could find that keeps ia64-hpux
> working.  Computing symbol_ref in ptr_mode and not Pmode is not a new
> thing, it's been done for quite some time.

Well yes, but this is neither ptr_mode nor Pmode (both are DImode on s390x).

Would changing the test in expand_expr_addr_expr from 

  if (!target.valid_pointer_mode (tmode))

to

  if (tmode != Pmode && tmode != ptr_mode)

be OK with you?  This should cause no change to ia64 ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Regression due to tree loop opt / expand weirdness

[Richard Henderson]

On Sun, Oct 03, 2004 at 09:40:12PM +0200, Ulrich Weigand wrote:
> Richard, I appears you've added the valid_pointer_mode check to 
> expand_expr_addr_expr -- could you elaborate how the back-end is
> supposed to handle this?

The reason to allow non-Pmode is that otherwise ilp32 on 64-bit targets
breaks.  That's the narrowest condition I could find that keeps ia64-hpux
working.  Computing symbol_ref in ptr_mode and not Pmode is not a new
thing, it's been done for quite some time.

As for why the tree loop optimizer is playing such silly narrowing games
with pointers, ask Zdenek.  I've already expressed my opinion that it's
wrong, but it has been an uphill fight trying to convince him.


r~

Regression due to tree loop opt / expand weirdness

[Ulrich Weigand]

Hello,

since several days the memcpy-2.c test case fails on s390x-ibm-linux.
The (reduced) test case is:

char buf[101];
void test (int len)
{
  char *p;
  int i;

  p = buf;
  for (i = 0; i < len; i++, p++)
    f (*p);
}

and its loop gets transformed by the tree-level loop optimizer into:

  # p_1 = PHI <p_12(6), &buf(3)>;
<L0>:;
  p_16 = p_1;
  D.1061_8 = *p_16;
  D.1062_9 = (int) D.1061_8;
  f (D.1062_9);
  p_4 = p_1 + 1B;
  p_12 = p_4;
  D.1092_15 = (unsigned int) p_12;
  D.1093_14 = (unsigned int) &buf;
  D.1094_13 = D.1092_15 - D.1093_14;
  i_5 = (int) D.1094_13;
  if (i_5 < len_7) goto <L9>; else goto <L8>;

The expander then transforms the D.1093 assigment into:

(insn 45 43 47 (set (reg:SI 61 [ D.1093 ])
        (symbol_ref:SI ("buf") <var_decl 0x20000286c40 buf>)) -1 (nil)
    (nil))

which subsequently ICEs as we cannot handle symbol_ref:SI in 64-bit mode.

Now there's a couple of odd things here.  First of all, the transformation
of the loop exit condition is really weird; even on 32-bit where compilation
is successful, this exit condition is so complicated that the RTL optimizers
don't manage to transform the loop into doloop form any more.


The other odd thing is why the expander attempts to generate a symbol_ref
with anything other than Pmode.  This appears to be done by the combination
of expand_expr_addr_expr and convert_memory_address, which can cause any
mode to appear in a symbol_ref that valid_pointer_mode accepts.

Now it is true that valid_pointer_mode accepts SImode on s390x, because
we want to allow the user to declare SImode pointer variables (this is
mainly useful for TPF).  What I expected this to achieve was to generate
conversion code whenever accessing a variable declared thus -- but all
internal use of addresses, in particular all symbolic address arithmetic
would continue to be performed in Pmode.

Richard, I appears you've added the valid_pointer_mode check to 
expand_expr_addr_expr -- could you elaborate how the back-end is
supposed to handle this?  Do we need to perform all the symbolic
address arithmetic (e.g. pic/tls address generation) in non-Pmode
in order to allow for non-Pmode user pointer variables?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de