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

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

[Robert Suchanek]

Hi Vladimir,

The following testcase fails when compiled with -O2 -mips32r2: 

long long a[];
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;
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];
}

It is reproducible on mips-linux-gnu using SVN revision 212763. After a patch 
to p5600.md the bug is not triggered but it may reappear in the future.

The decompose_normal_address function throws an assertion because it cannot
decompose the following RTL:

(mem/c:SI (lo_sum:SI (high:SI (symbol_ref:SI ("w")))  
                (const:SI (plus:SI (symbol_ref:SI ("w"))                        
                        (const_int 4 [0x4]))

It appears that it all starts in the following instruction and how LRA deals
with REG_EQUIV notes:

(insn 107 119 123 10 (set (reg:SI 283)                                                         
        (ior:SI (reg:SI 284 [ a+12 ])                                                          
            (reg:SI 309 [ D.1467+4 ]))) init.i:16 163 {*iorsi3}                                
     (expr_list:REG_DEAD (reg:SI 309 [ D.1467+4 ])                                             
        (expr_list:REG_DEAD (reg:SI 284 [ a+12 ])                                              
            (expr_list:REG_EQUIV (mem/c:SI (lo_sum:SI (reg/f:SI 274)                           
                        (const:SI (plus:SI (symbol_ref:SI ("w"))  
                                (const_int 4 [0x4])))))                         
                (nil)))))

There are two conditions necessary to trigger the ICE.
Firstly, the pseudo 274 is spilled to memory that is marked by IRA, thus, 
during LRA pass the pseudo 274 is replaced with 'high' when equivalences 
are updated for pseudos. Secondly, pseudo 283 also gets spilled and LRA starts 
using equivalences but LO_SUM and HIGH are already combined leading to 
an assertion error. 

Accepting HIGH as the base does seem to solve the problem. HIGH is also 
reloaded after the decomposition splitting HIGH/LO_SUM into a pair again. 
However, is this an acceptable solution?

Regards,
Robert

gcc/
	* rtlanal.c (get_base_term): Accept HIGH as the base term.


diff --git gcc/rtlanal.c gcc/rtlanal.c
index 82cfc1bf..2bea2ca 100644
--- gcc/rtlanal.c
+++ gcc/rtlanal.c
@@ -5624,6 +5624,7 @@ get_base_term (rtx *inner)
     inner = strip_address_mutations (&XEXP (*inner, 0));
   if (REG_P (*inner)
       || MEM_P (*inner)
+      || GET_CODE (*inner) == HIGH
       || GET_CODE (*inner) == SUBREG)
     return inner;
   return 0;

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

[Steven Bosscher]

On Fri, Aug 15, 2014 at 5:45 PM, Robert Suchanek wrote:
> gcc/
>         * rtlanal.c (get_base_term): Accept HIGH as the base term.
>
>
> diff --git gcc/rtlanal.c gcc/rtlanal.c
> index 82cfc1bf..2bea2ca 100644
> --- gcc/rtlanal.c
> +++ gcc/rtlanal.c
> @@ -5624,6 +5624,7 @@ get_base_term (rtx *inner)
>      inner = strip_address_mutations (&XEXP (*inner, 0));
>    if (REG_P (*inner)
>        || MEM_P (*inner)
> +      || GET_CODE (*inner) == HIGH
>        || GET_CODE (*inner) == SUBREG)
>      return inner;
>    return 0;

This is not correct, BASE is a *variable* expression, HIGH is a
*constant* expression.

It's hard to say what the correct fix should be, but it sounds like
the address you get after the substitutions should be simplified
(folded).

B.R.,
Steven

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

[Robert Suchanek]

Hi Steven/Vladimir,

> It's hard to say what the correct fix should be, but it sounds like
> the address you get after the substitutions should be simplified
> (folded).

Coming back to the original testcase and re-analyzing the problem, it appears
that there is, indeed, a missing case for simplification of LO_SUM/HIGH pair.
The patch attached resolves the issue.

Although the testcase is not reproducible on the trunk, I think it is still
worth to include it in case the ICE reoccurred.

The patch has been bootstrapped and regtested on x86_64-unknown-linux-gnu target
and similarly tested against SVN revision r212763 where it can be reproduced.

Regards,
Robert

2015-01-08  Robert Suchanek  <robert.suchanek@imgtec.com>

gcc/
	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high x)
	(const (plus x offset))) to (const (plus x offset)).

gcc/testsuite/
	* gcc.target/mips/20150108.c: New test.
---
 gcc/simplify-rtx.c                       |    6 ++++++
 gcc/testsuite/gcc.target/mips/20150108.c |   25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/mips/20150108.c

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 04af01e..7621316 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -503,6 +503,12 @@ simplify_replace_fn_rtx (rtx x, const_rtx old_rtx,
 	  if (GET_CODE (op0) == HIGH && rtx_equal_p (XEXP (op0, 0), op1))
 	    return op1;
 
+	  /* (lo_sum (high x) (const (plus x ofs))) -> (const (plus x ofs))  */
+	  if (GET_CODE (op0) == HIGH && GET_CODE (op1) == CONST
+	      && GET_CODE(XEXP (op1, 0)) == PLUS
+	      && rtx_equal_p (XEXP (XEXP (op1, 0), 0), XEXP (op0, 0)))
+	    return op1;
+
 	  if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1))
 	    return x;
 	  return gen_rtx_LO_SUM (mode, op0, op1);
diff --git a/gcc/testsuite/gcc.target/mips/20150108.c b/gcc/testsuite/gcc.target/mips/20150108.c
new file mode 100644
index 0000000..f18dbe7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/20150108.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-mips32r2" } */
+
+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;
+
+NOMIPS16 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];
+}
-- 
1.7.9.5

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

[Matthew Fortune]

Robert Suchanek <Robert.Suchanek@imgtec.com> writes:

> gcc/
> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high x)
> 	(const (plus x offset))) to (const (plus x offset)).

The fix appears valid to me. Just some comments on the test case.

> a/gcc/testsuite/gcc.target/mips/20150108.c
> b/gcc/testsuite/gcc.target/mips/20150108.c
> new file mode 100644
> index 0000000..f18dbe7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/20150108.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mips32r2" } */

Please remove this line as there is nothing ISA dependent in the test case.

> +
> +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;
> +

nit, no return type specified.

> +NOMIPS16 fn1() {

Nit, newline for the brace.

> +  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];
> +}
> --
> 1.7.9.5

Thanks,
Matthew

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

[pinskia]

> On Jan 9, 2015, at 4:20 AM, Matthew Fortune <Matthew.Fortune@imgtec.com> wrote:
> 
> Robert Suchanek <Robert.Suchanek@imgtec.com> writes:
> 
>> gcc/
>>    * simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high x)
>>    (const (plus x offset))) to (const (plus x offset)).
> 
> The fix appears valid to me. Just some comments on the test case.
> 
>> a/gcc/testsuite/gcc.target/mips/20150108.c
>> b/gcc/testsuite/gcc.target/mips/20150108.c
>> new file mode 100644
>> index 0000000..f18dbe7
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/mips/20150108.c
>> @@ -0,0 +1,25 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-mips32r2" } */
> 
> Please remove this line as there is nothing ISA dependent in the test case.

And since there is no mips specific part to the testcase (except for nomips16), we should place it in the generic part of the testsuite. 

Thanks,
Andrew

> 
>> +
>> +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;
>> +
> 
> nit, no return type specified.
> 
>> +NOMIPS16 fn1() {
> 
> Nit, newline for the brace.
> 
>> +  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];
>> +}
>> --
>> 1.7.9.5
> 
> Thanks,
> Matthew

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

[Jeff Law]

On 01/09/15 04:32, Robert Suchanek wrote:
> Hi Steven/Vladimir,
>
>> It's hard to say what the correct fix should be, but it sounds like
>> the address you get after the substitutions should be simplified
>> (folded).
>
> Coming back to the original testcase and re-analyzing the problem, it appears
> that there is, indeed, a missing case for simplification of LO_SUM/HIGH pair.
> The patch attached resolves the issue.
>
> Although the testcase is not reproducible on the trunk, I think it is still
> worth to include it in case the ICE reoccurred.
>
> The patch has been bootstrapped and regtested on x86_64-unknown-linux-gnu target
> and similarly tested against SVN revision r212763 where it can be reproduced.
>
> Regards,
> Robert
>
> 2015-01-08  Robert Suchanek  <robert.suchanek@imgtec.com>
>
> gcc/
> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high x)
> 	(const (plus x offset))) to (const (plus x offset)).
You have to be careful here.  Whether or not this transformation is 
valid depends on the size of the offset and whether or not the port has 
an overlap between its sethi and losum insns and whether or not any 
rounding occurs when applying the relocations for sethi/losum as well as 
potentially other factors.

We don't currently have a way for ports to indicate what offsets would 
make this kind of simplification valid.   In fact, there's at least one 
port (PA) where the validity of this kind of simplification can't be 
determined until after LRA/reload when you know the full context of how 
the result is going to be used.

Jeff

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

[Richard Sandiford]

Jeff Law <law@redhat.com> writes:
> On 01/09/15 04:32, Robert Suchanek wrote:
>> Hi Steven/Vladimir,
>>
>>> It's hard to say what the correct fix should be, but it sounds like
>>> the address you get after the substitutions should be simplified
>>> (folded).
>>
>> Coming back to the original testcase and re-analyzing the problem, it appears
>> that there is, indeed, a missing case for simplification of LO_SUM/HIGH pair.
>> The patch attached resolves the issue.
>>
>> Although the testcase is not reproducible on the trunk, I think it is still
>> worth to include it in case the ICE reoccurred.
>>
>> The patch has been bootstrapped and regtested on
>> x86_64-unknown-linux-gnu target
>> and similarly tested against SVN revision r212763 where it can be reproduced.
>>
>> Regards,
>> Robert
>>
>> 2015-01-08  Robert Suchanek  <robert.suchanek@imgtec.com>
>>
>> gcc/
>> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high x)
>> 	(const (plus x offset))) to (const (plus x offset)).
> You have to be careful here.  Whether or not this transformation is 
> valid depends on the size of the offset and whether or not the port has 
> an overlap between its sethi and losum insns and whether or not any 
> rounding occurs when applying the relocations for sethi/losum as well as 
> potentially other factors.
>
> We don't currently have a way for ports to indicate what offsets would 
> make this kind of simplification valid.   In fact, there's at least one 
> port (PA) where the validity of this kind of simplification can't be 
> determined until after LRA/reload when you know the full context of how 
> the result is going to be used.

I agree this is the kind of thing we'd need to consider if we were
deciding whether it's valid to connect a (lo_sum (high x+N) x+N) to an
existing (high x).  But this code is handling cases where the connection
has already been made and we're trying to simplify the result.  Would it
be valid RTL to use:

  (lo_sum (high x) (const (plus x offset)))

to mean anything other than x+offset?

Hmm, admittedly the documentation doesn't guarantee that...

If we do go for the change, I think we should generalise it to handle
(lo_sum (high x+N) x+N') and (lo_sum (high x-N) x) too.  Things like
get_related_value or split_const could help there.

Thanks,
Richard

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

[Matthew Fortune]

Richard Sandiford <rdsandiford@googlemail.com>  writes:
> Jeff Law <law@redhat.com> writes:
> > On 01/09/15 04:32, Robert Suchanek wrote:
> >> Hi Steven/Vladimir,
> >>
> >>> It's hard to say what the correct fix should be, but it sounds like
> >>> the address you get after the substitutions should be simplified
> >>> (folded).
> >>
> >> Coming back to the original testcase and re-analyzing the problem, it
> >> appears that there is, indeed, a missing case for simplification of
> LO_SUM/HIGH pair.
> >> The patch attached resolves the issue.
> >>
> >> Although the testcase is not reproducible on the trunk, I think it is
> >> still worth to include it in case the ICE reoccurred.
> >>
> >> The patch has been bootstrapped and regtested on
> >> x86_64-unknown-linux-gnu target and similarly tested against SVN
> >> revision r212763 where it can be reproduced.
> >>
> >> Regards,
> >> Robert
> >>
> >> 2015-01-08  Robert Suchanek  <robert.suchanek@imgtec.com>
> >>
> >> gcc/
> >> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high
> x)
> >> 	(const (plus x offset))) to (const (plus x offset)).
> > You have to be careful here.  Whether or not this transformation is
> > valid depends on the size of the offset and whether or not the port
> > has an overlap between its sethi and losum insns and whether or not
> > any rounding occurs when applying the relocations for sethi/losum as
> > well as potentially other factors.
> >
> > We don't currently have a way for ports to indicate what offsets would
> > make this kind of simplification valid.   In fact, there's at least
> one
> > port (PA) where the validity of this kind of simplification can't be
> > determined until after LRA/reload when you know the full context of
> > how the result is going to be used.
> 
> I agree this is the kind of thing we'd need to consider if we were
> deciding whether it's valid to connect a (lo_sum (high x+N) x+N) to an
> existing (high x).  But this code is handling cases where the connection
> has already been made and we're trying to simplify the result.  Would it
> be valid RTL to use:
> 
>   (lo_sum (high x) (const (plus x offset)))
> 
> to mean anything other than x+offset?
> 
> Hmm, admittedly the documentation doesn't guarantee that...

I guess so. I took the phrasing below for (high:m exp) to mean that high
only made sense when used with lo_sum.

"It is used with lo_sum to represent the typical two-instruction sequence
used in RISC machines to reference a global memory location"

I don't know if that was the intended meaning though. There is nothing
to state that lo_sum has to be used with high though so lo_sum could
be applied to a different base quite legitimately it seems.

> If we do go for the change, I think we should generalise it to handle
> (lo_sum (high x+N) x+N') and (lo_sum (high x-N) x) too.  Things like
> get_related_value or split_const could help there.

Seems reasonable to cover these cases but I think the code already does
that (this is directly above the new case Robert added):

	  /* (lo_sum (high x) x) -> x  */
	  if (GET_CODE (op0) == HIGH && rtx_equal_p (XEXP (op0, 0), op1))
 	    return op1;

So, (lo_sum (high x) x) is already simplified to x but the special case
where a constant is only added to the lo_sum operand is missing. What is
likely to happen overall is:

(lo_sum (high x) (const (plus x ofs))) -> (const (plus x ofs))

Followed by something splitting the simplified result because it is not
valid and getting:

(set (reg) (const (plus x ofs)))
->
(set (reg) (high (const (plus x ofs))))
(set (reg) (lo_sum (reg) (const (plus x ofs))))

The code that combined high/lo_sum has to validate the result and this
will generally mean it has to be split again I expect.

Thanks,
Matthew

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

[Richard Sandiford]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> Richard Sandiford <rdsandiford@googlemail.com>  writes:
>> Jeff Law <law@redhat.com> writes:
>> > On 01/09/15 04:32, Robert Suchanek wrote:
>> >> Hi Steven/Vladimir,
>> >>
>> >>> It's hard to say what the correct fix should be, but it sounds like
>> >>> the address you get after the substitutions should be simplified
>> >>> (folded).
>> >>
>> >> Coming back to the original testcase and re-analyzing the problem, it
>> >> appears that there is, indeed, a missing case for simplification of
>> LO_SUM/HIGH pair.
>> >> The patch attached resolves the issue.
>> >>
>> >> Although the testcase is not reproducible on the trunk, I think it is
>> >> still worth to include it in case the ICE reoccurred.
>> >>
>> >> The patch has been bootstrapped and regtested on
>> >> x86_64-unknown-linux-gnu target and similarly tested against SVN
>> >> revision r212763 where it can be reproduced.
>> >>
>> >> Regards,
>> >> Robert
>> >>
>> >> 2015-01-08  Robert Suchanek  <robert.suchanek@imgtec.com>
>> >>
>> >> gcc/
>> >> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum (high
>> x)
>> >> 	(const (plus x offset))) to (const (plus x offset)).
>> > You have to be careful here.  Whether or not this transformation is
>> > valid depends on the size of the offset and whether or not the port
>> > has an overlap between its sethi and losum insns and whether or not
>> > any rounding occurs when applying the relocations for sethi/losum as
>> > well as potentially other factors.
>> >
>> > We don't currently have a way for ports to indicate what offsets would
>> > make this kind of simplification valid.   In fact, there's at least
>> one
>> > port (PA) where the validity of this kind of simplification can't be
>> > determined until after LRA/reload when you know the full context of
>> > how the result is going to be used.
>> 
>> I agree this is the kind of thing we'd need to consider if we were
>> deciding whether it's valid to connect a (lo_sum (high x+N) x+N) to an
>> existing (high x).  But this code is handling cases where the connection
>> has already been made and we're trying to simplify the result.  Would it
>> be valid RTL to use:
>> 
>>   (lo_sum (high x) (const (plus x offset)))
>> 
>> to mean anything other than x+offset?
>> 
>> Hmm, admittedly the documentation doesn't guarantee that...
>
> I guess so. I took the phrasing below for (high:m exp) to mean that high
> only made sense when used with lo_sum.
>
> "It is used with lo_sum to represent the typical two-instruction sequence
> used in RISC machines to reference a global memory location"
>
> I don't know if that was the intended meaning though. There is nothing
> to state that lo_sum has to be used with high though so lo_sum could
> be applied to a different base quite legitimately it seems.

Yeah, it's also used for small-data accesses, where the base is the
global pointer.  You can also add a variable offset in between the
high and lo_sum, which is the main reason we can't just take the
second operand of the lo_sum as golden.  Seeing the high is important.

>> If we do go for the change, I think we should generalise it to handle
>> (lo_sum (high x+N) x+N') and (lo_sum (high x-N) x) too.  Things like
>> get_related_value or split_const could help there.
>
> Seems reasonable to cover these cases but I think the code already does
> that (this is directly above the new case Robert added):
>
> 	  /* (lo_sum (high x) x) -> x  */
> 	  if (GET_CODE (op0) == HIGH && rtx_equal_p (XEXP (op0, 0), op1))
>  	    return op1;
>
> So, (lo_sum (high x) x) is already simplified to x but the special case
> where a constant is only added to the lo_sum operand is missing.

No, the cases I was describing were where the high is a (const (plus x -N))
and the lo_sum is just x, or where the high is (const (plus x N)) and the
lo_sum is (const (plus x N')).

I.e. rather than have the two special cases of (lo_sum (high x) x)
and (lo_sum (high x) (const (plus x N))), we should handle all cases
of (lo_sum (high x) y) in which y and x have the same base.

E.g. if we have an int array called "x" that is aligned to 64 bits,
we can access x[3] from the same high as x[2].  Neither the existing
code nor the patch would be able to simplify that back to x[3].

Thanks,
Richard

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

[Matthew Fortune]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> > Richard Sandiford <rdsandiford@googlemail.com>  writes:
> >> Jeff Law <law@redhat.com> writes:
> >> > On 01/09/15 04:32, Robert Suchanek wrote:
> >> >> Hi Steven/Vladimir,
> >> >>
> >> >>> It's hard to say what the correct fix should be, but it sounds
> >> >>> like the address you get after the substitutions should be
> >> >>> simplified (folded).
> >> >>
> >> >> Coming back to the original testcase and re-analyzing the problem,
> >> >> it appears that there is, indeed, a missing case for
> >> >> simplification of
> >> LO_SUM/HIGH pair.
> >> >> The patch attached resolves the issue.
> >> >>
> >> >> Although the testcase is not reproducible on the trunk, I think it
> >> >> is still worth to include it in case the ICE reoccurred.
> >> >>
> >> >> The patch has been bootstrapped and regtested on
> >> >> x86_64-unknown-linux-gnu target and similarly tested against SVN
> >> >> revision r212763 where it can be reproduced.
> >> >>
> >> >> Regards,
> >> >> Robert
> >> >>
> >> >> 2015-01-08  Robert Suchanek  <robert.suchanek@imgtec.com>
> >> >>
> >> >> gcc/
> >> >> 	* simplify-rtx.c (simplify_replace_fn_rtx): Simplify (lo_sum
> >> >> (high
> >> x)
> >> >> 	(const (plus x offset))) to (const (plus x offset)).
> >> > You have to be careful here.  Whether or not this transformation is
> >> > valid depends on the size of the offset and whether or not the port
> >> > has an overlap between its sethi and losum insns and whether or not
> >> > any rounding occurs when applying the relocations for sethi/losum
> >> > as well as potentially other factors.
> >> >
> >> > We don't currently have a way for ports to indicate what offsets
> would
> >> > make this kind of simplification valid.   In fact, there's at least
> >> one
> >> > port (PA) where the validity of this kind of simplification can't
> >> > be determined until after LRA/reload when you know the full context
> >> > of how the result is going to be used.
> >>
> >> I agree this is the kind of thing we'd need to consider if we were
> >> deciding whether it's valid to connect a (lo_sum (high x+N) x+N) to
> >> an existing (high x).  But this code is handling cases where the
> >> connection has already been made and we're trying to simplify the
> >> result.  Would it be valid RTL to use:
> >>
> >>   (lo_sum (high x) (const (plus x offset)))
> >>
> >> to mean anything other than x+offset?
> >>
> >> Hmm, admittedly the documentation doesn't guarantee that...
> >
> > I guess so. I took the phrasing below for (high:m exp) to mean that
> > high only made sense when used with lo_sum.
> >
> > "It is used with lo_sum to represent the typical two-instruction
> > sequence used in RISC machines to reference a global memory location"
> >
> > I don't know if that was the intended meaning though. There is nothing
> > to state that lo_sum has to be used with high though so lo_sum could
> > be applied to a different base quite legitimately it seems.
> 
> Yeah, it's also used for small-data accesses, where the base is the
> global pointer.  You can also add a variable offset in between the high
> and lo_sum, which is the main reason we can't just take the second
> operand of the lo_sum as golden.  Seeing the high is important.
> 
> >> If we do go for the change, I think we should generalise it to handle
> >> (lo_sum (high x+N) x+N') and (lo_sum (high x-N) x) too.  Things like
> >> get_related_value or split_const could help there.
> >
> > Seems reasonable to cover these cases but I think the code already
> > does that (this is directly above the new case Robert added):
> >
> > 	  /* (lo_sum (high x) x) -> x  */
> > 	  if (GET_CODE (op0) == HIGH && rtx_equal_p (XEXP (op0, 0), op1))
> >  	    return op1;
> >
> > So, (lo_sum (high x) x) is already simplified to x but the special
> > case where a constant is only added to the lo_sum operand is missing.
> 
> No, the cases I was describing were where the high is a (const (plus x -
> N)) and the lo_sum is just x, or where the high is (const (plus x N))
> and the lo_sum is (const (plus x N')).
> 
> I.e. rather than have the two special cases of (lo_sum (high x) x) and
> (lo_sum (high x) (const (plus x N))), we should handle all cases of
> (lo_sum (high x) y) in which y and x have the same base.
> 
> E.g. if we have an int array called "x" that is aligned to 64 bits, we
> can access x[3] from the same high as x[2].  Neither the existing code
> nor the patch would be able to simplify that back to x[3].

I see. At first glance the other cases seem like riskier simplifications
but they are probably no different in reality.

The simplification would presumably just end up as the second operand
to lo_sum in all cases regardless of whether there was a constant added/
subtracted from the high part value. Something along these lines:

if (GET_CODE (op0) == HIGH)
  {
    rtx base0, base1;
    base0 = get_related_value (XEXP (op0, 0));
    if (!base0)
      base0 = XEXP (op0, 0);
    base1 = get_related_value (op1);
    if (!base1)
      base1 = op1;
    if (rtx_equal_p (base0, base1)
      return op1;
  }

Thanks,
Matthew

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

[Jeff Law]

On 01/10/15 05:51, Richard Sandiford wrote:
>
> I agree this is the kind of thing we'd need to consider if we were
> deciding whether it's valid to connect a (lo_sum (high x+N) x+N) to an
> existing (high x).  But this code is handling cases where the connection
> has already been made and we're trying to simplify the result.  Would it
> be valid RTL to use:
And I probably should have said that if we've already somehow determined 
that the high/losum are paired then it'd be OK.  I hesitated simply 
because I hadn't walked how this code was called or used in any detail.

>
>    (lo_sum (high x) (const (plus x offset)))
>
> to mean anything other than x+offset?
Agreed, when we've made a connection between the two -- for example, if 
we know the output of the high dominates and was originally used in the 
lo_sum.   However, we can't make that assumption for an arbitrary 
high/lo_sum, even if they refer to the same x.


> If we do go for the change, I think we should generalise it to handle
> (lo_sum (high x+N) x+N') and (lo_sum (high x-N) x) too.  Things like
> get_related_value or split_const could help there.
Unless the change is pretty trivial, this might be a gcc-6 kind of thing.

jeff

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

[Jeff Law]

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

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

[Robert Suchanek]

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;
+

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

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

[Richard Sandiford]

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

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

[Jeff Law]

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

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

[Robert Suchanek]

> 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

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

[Matthew Fortune]

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

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

[Markus Trippelsdorf]

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.

-- 
Markus

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

[Markus Trippelsdorf]

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.

-- 
Markus

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

[Matthew Fortune]

> 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