[RFC,LRA,MIPS] ICE: in decompose_normal_address, at rtlanal.c:5817

Here is the revised patch that would handle the other cases as per Richard's
comments.

I slightly modified Matthew's proposed patch and used split_const
instead of get_related_value. AFAICS, the canonical form would always have
the 'plus' expression.

The offset on the high part is most likely not important as the code generation
has to guarantee that the low part represents the true address in the case 
where the high and lo_sum are directly related. 

Regards,
Robert

gcc/
	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum
	(high x) y) to y if x and y have the same base.

gcc/testsuite/
	* gcc.c-torture/compile/20150108.c: New test.


> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: 12 January 2015 18:56
> To: Matthew Fortune; Richard Sandiford
> Cc: Robert Suchanek; Steven Bosscher; vmakarov@redhat.com; gcc-
> patches@gcc.gnu.org
> Subject: Re: [RFC, PATCH][LRA, MIPS] ICE: in decompose_normal_address, at
> rtlanal.c:5817
> 
> On 01/10/15 09:35, Matthew Fortune wrote:
> >
> > I guess so. I took the phrasing below for (high:m exp) to mean that high
> > only made sense when used with lo_sum.
> True.  But one can use a single high with different lo_sum expressions
> when those lo_sum expressions are related.
> 
> So you might have a single high such as
> 
> (high (symbol_ref "x"))
> 
> That feeds multiple lo_sum expressions like
> 
> (lo_sum (reg) (symbol_ref "x"))
> (lo_sum (reg) (const (plus (symbol_ref "x") (const_int 4))))
> (lo_sum (reg) (const (plus (symbol_ref "x") (const_int 8))))
> (lo_sum (reg) (const (plus (symbol_ref "x") (const_int 12)))
> 
> 
> IIRC this gets implemented in either the move expander or a
> legitimize_address hook.  You start with a high/lo_sum pair for each
> reference.  However, you rewrite the high part to chop off low bits.
> That makes many of the high expressions become common subexpressions and
> they get removed by CSE in the expected ways.
> 
> You have to be careful for overflows and such.  I don't recall the
> precise rules there, but it was the source of problems with interfacing
> with the optimizing PA linker.
> 
> 
> Jeff

Robert Suchanek <Robert.Suchanek@imgtec.com> writes:
> Here is the revised patch that would handle the other cases as per Richard's
> comments.
>
> I slightly modified Matthew's proposed patch and used split_const
> instead of get_related_value. AFAICS, the canonical form would always have
> the 'plus' expression.
>
> The offset on the high part is most likely not important as the code generation
> has to guarantee that the low part represents the true address in the case 
> where the high and lo_sum are directly related. 

This looks good to me FWIW.

Thanks,
Richard

On 01/14/15 10:10, Robert Suchanek wrote:
> Here is the revised patch that would handle the other cases as per Richard's
> comments.
>
> I slightly modified Matthew's proposed patch and used split_const
> instead of get_related_value. AFAICS, the canonical form would always have
> the 'plus' expression.
>
> The offset on the high part is most likely not important as the code generation
> has to guarantee that the low part represents the true address in the case
> where the high and lo_sum are directly related.
>
> Regards,
> Robert
>
> gcc/
> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum
> 	(high x) y) to y if x and y have the same base.
>
> gcc/testsuite/
> 	* gcc.c-torture/compile/20150108.c: New test.
OK.  The MIPS and Sparc ports are probably going to hit this the 
hardest.  So you've got a vested interest in dealing with any fallout :-)

jeff

> OK.  The MIPS and Sparc ports are probably going to hit this the
> hardest.  So you've got a vested interest in dealing with any fallout :-)
> 
> jeff

That's fine. The MIPS port has been widely tested and I cross tested it on
sparc-linux-gnu target so hopefully there won't any fallout.

Robert

> > OK.  The MIPS and Sparc ports are probably going to hit this the
> > hardest.  So you've got a vested interest in dealing with any fallout
> > :-)
> >
> > jeff
> 
> That's fine. The MIPS port has been widely tested and I cross tested it
> on sparc-linux-gnu target so hopefully there won't any fallout.

Committed as r219729 on Robert's behalf.

Robert has now requested write access which I have approved.

Thanks,
Matthew

On 2015.01.14 at 17:10 +0000, Robert Suchanek wrote:
> Here is the revised patch that would handle the other cases as per Richard's
> comments.
> 
> I slightly modified Matthew's proposed patch and used split_const
> instead of get_related_value. AFAICS, the canonical form would always have
> the 'plus' expression.
> 
> The offset on the high part is most likely not important as the code generation
> has to guarantee that the low part represents the true address in the case 
> where the high and lo_sum are directly related. 
> 
> Regards,
> Robert
> 
> gcc/
> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum
> 	(high x) y) to y if x and y have the same base.
> 
> gcc/testsuite/
> 	* gcc.c-torture/compile/20150108.c: New test.
> 
> diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> index 04af01e..df86f8b 100644
> --- a/gcc/simplify-rtx.c
> +++ b/gcc/simplify-rtx.c
> @@ -499,9 +499,15 @@ simplify_replace_fn_rtx (rtx x, const_rtx old_rtx,
>  	  op0 = simplify_replace_fn_rtx (XEXP (x, 0), old_rtx, fn, data);
>  	  op1 = simplify_replace_fn_rtx (XEXP (x, 1), old_rtx, fn, data);
>  
> -	  /* (lo_sum (high x) x) -> x  */
> -	  if (GET_CODE (op0) == HIGH && rtx_equal_p (XEXP (op0, 0), op1))
> -	    return op1;
> +	  /* (lo_sum (high x) y) -> y where x and y have the same base.  */
> +	  if (GET_CODE (op0) == HIGH)
> +	    {
> +	      rtx base0, base1, offset0, offset1;
> +	      split_const (XEXP (op0, 0), &base0, &offset0);
> +	      split_const (op1, &base1, &offset1);
> +	      if (rtx_equal_p (base0, base1))
> +		return op1;
> +	    }
>  
>  	  if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1))
>  	    return x;
> diff --git a/gcc/testsuite/gcc.c-torture/compile/20150108.c b/gcc/testsuite/gcc.c-torture/compile/20150108.c
> new file mode 100644
> index 0000000..15c53e3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/20150108.c
> @@ -0,0 +1,23 @@
> +long long a[10];
> +long long b, c, d, k, m, n, o, p, q, r, s, t, u, v, w;
> +int e, f, g, h, i, j, l, x;
> +
> +int fn1 () {
> +  for (; x; x++)
> +    if (x & 1)
> +      s = h | g;
> +    else
> +      s = f | e;
> +  l = ~0;
> +  m = 1 | k;
> +  n = i;
> +  o = j;
> +  p = f | e;
> +  q = h | g;
> +  w = d | c | a[1];
> +  t = c;
> +  v = b | c;
> +  u = v;
> +  r = b | a[4];
> +  return e;
> +

There is a missing } in the testcase.

On 2015.01.16 at 14:56 +0100, Markus Trippelsdorf wrote:
> On 2015.01.14 at 17:10 +0000, Robert Suchanek wrote:
> > +  u = v;
> > +  r = b | a[4];
> > +  return e;
> > +
> 
> There is a missing } in the testcase.

Fixed in r219740 as obvious.

> On 2015.01.16 at 14:56 +0100, Markus Trippelsdorf wrote:
> > On 2015.01.14 at 17:10 +0000, Robert Suchanek wrote:
> > > +  u = v;
> > > +  r = b | a[4];
> > > +  return e;
> > > +
> >
> > There is a missing } in the testcase.
> 
> Fixed in r219740 as obvious.

Thanks Markus.

Sorry about that, I must have broken it copying and pasting the patch. I only
did a build of the compiler before commit.

Matthew