PR middle-end/103059: reload: Also accept ASHIFT with indexed addressing

  Correct a `vax-netbsdelf' target regression ultimately caused by commit 
c605a8bf9270 ("VAX: Accept ASHIFT in address expressions") (needed for 
LRA) and as of commit 4a960d548b7d ("Avoid invalid loop transformations 
in jump threading registry.") causing a build error in libgcc:

.../libgcc/libgcov-driver.c: In function 'gcov_do_dump':
.../libgcc/libgcov-driver.c:686:1: error: insn does not satisfy its constraints:
  686 | }
      | ^
(insn 2051 2050 2052 185 (set (reg/f:SI 0 %r0 [555])
        (plus:SI (ashift:SI (mem/c:SI (plus:SI (reg/f:SI 13 %fp)
                        (const_int -28 [0xffffffffffffffe4])) [40 %sfp+-28 S4 A32])
                (const_int 3 [0x3]))
            (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
                (const_int 24 [0x18])))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
     (nil))
during RTL pass: postreload
.../libgcc/libgcov-driver.c:686:1: internal compiler error: in extract_constrain_insn, at recog.c:2670
0x1122a5ff _fatal_insn(char const*, rtx_def const*, char const*, int, char const*)
	.../gcc/rtl-error.c:108
0x1122a697 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
	.../gcc/rtl-error.c:118
0x111b5f2f extract_constrain_insn(rtx_insn*)
	.../gcc/recog.c:2670
0x11143eef reload_cse_simplify_operands
	.../gcc/postreload.c:407
0x11142fdb reload_cse_simplify
	.../gcc/postreload.c:132
0x11143533 reload_cse_regs_1
	.../gcc/postreload.c:238
0x11142ce7 reload_cse_regs
	.../gcc/postreload.c:66
0x1114af33 execute
	.../gcc/postreload.c:2355
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

This is because reload does not recognize the ASHIFT form of scaled 
indexed addressing that the offending commit enabled the backend to 
produce, and as seen in the RTL above lets the pseudo holding the 
index part become the original memory reference rather than reloading it 
into a hard register.

Fix it then by adding said form to reload, removing the build failure 
and numerous similar regressions throughout `vax-netbsdelf' test suites 
run with the source as at right before the build regression.

Cf. <https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567256.html>, 
and commit 6b3034eaba83 ("lra: Canonicalize mult to shift in address 
reloads").

	gcc/
	PR middle-end/103059
	* reload.c (find_reloads_address_1): Also accept the ASHIFT form 
	of indexed addressing.
	(find_reloads): Adjust accordingly.
---
Hi,

 NB the change to `find_reloads_address_1' is one that removes the build 
error and I am fairly sure I have nailed it.  The change to `find_reloads' 
seemed a natural consequence then as surely it deals with the results from 
`find_reloads_address_1', though things appear to work regardless and I 
haven't investigated what difference it makes.

 I have not yet tracked down which change after commit c605a8bf9270 made 
regressions appear in the test suites, however clearly the commit wasn't 
as complete a change as it should have been.  I'll see if I can find it 
and will mention it in the final commit description if there is anything 
useful in that.

 As noted in the commit description this has been regression-tested with 
commit 4a960d548b7d^.  I'm running regression-testing with GCC 11 right 
now as well and expect results by the end of week.

 I was trying to chase another target I could use to regression-test this 
with that does do scaled indexed addressing while still using old reload.  
The i386 port would be a good candidate, but it has switched to LRA long 
ago with no option to use old reload, and I think there would be little 
point in adding one just for the sake of such verification.  Do we have 
any other port actually that could be affected by this change?

 OK to apply to trunk then and, as a stable regression, to GCC 11?

  Maciej
---
 gcc/reload.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

gcc-find-reloads-address-ashift.diff
  

On 11/3/2021 7:53 AM, Maciej W. Rozycki wrote:
> Correct a `vax-netbsdelf' target regression ultimately caused by commit
> c605a8bf9270 ("VAX: Accept ASHIFT in address expressions") (needed for
> LRA) and as of commit 4a960d548b7d ("Avoid invalid loop transformations
> in jump threading registry.") causing a build error in libgcc:
But within a MEM the ASHIFT should have been canonicalized into a MULT 
by an appropriate power of two according to the canonicalization rules.


>
> .../libgcc/libgcov-driver.c: In function 'gcov_do_dump':
> .../libgcc/libgcov-driver.c:686:1: error: insn does not satisfy its constraints:
>    686 | }
>        | ^
> (insn 2051 2050 2052 185 (set (reg/f:SI 0 %r0 [555])
>          (plus:SI (ashift:SI (mem/c:SI (plus:SI (reg/f:SI 13 %fp)
>                          (const_int -28 [0xffffffffffffffe4])) [40 %sfp+-28 S4 A32])
>                  (const_int 3 [0x3]))
>              (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
>                  (const_int 24 [0x18])))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
>       (nil))
I'm guessing this insn is the result of reloading an address within a 
MEM into a REG.   Had that address been in a canonical form I don't 
think this patch would be needed.

Am I missing something?

jeff
  

On Thu, 4 Nov 2021, Jeff Law wrote:

> On 11/3/2021 7:53 AM, Maciej W. Rozycki wrote:
> > Correct a `vax-netbsdelf' target regression ultimately caused by commit
> > c605a8bf9270 ("VAX: Accept ASHIFT in address expressions") (needed for
> > LRA) and as of commit 4a960d548b7d ("Avoid invalid loop transformations
> > in jump threading registry.") causing a build error in libgcc:
> But within a MEM the ASHIFT should have been canonicalized into a MULT by an
> appropriate power of two according to the canonicalization rules.

 I thought so as well, but was straigtened out by Richard:

On Thu, 25 Mar 2021, Richard Sandiford wrote:

> > [From <https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552586.html>; 
> > also commit 6b3034eaba83.]
> >
> >  Guys, this triggers a backend's functional regression and an ICE in the 
> > test suite with the LRA conversion I'm currently working on for the VAX 
> > backend.  Before I go ahead and paper it over in the backend I'd like to 
> > understand why this change was considered correct in the first place.
> >
> >  Contrary to what the change description suggests the ASHIFT form is not 
> > documented to be the canonical form for constant multiplication involving 
> > a power of two for addresses used outside `mem'.
> 
> One thing to note here is that, outside of a mem, there's no distinction
> between an address calculation and normal integer arithmetic.  In other
> words, “addresses used outside of a ‘mem’” aren't a distinct category of
> rtx that can be separated from other things outside of a “mem“.  So…
> 
> > What our rules only say 
> > is that for addresses inside `mem' the MULT form is:
> >
> >    * Within address computations (i.e., inside 'mem'), a left shift is
> >      converted into the appropriate multiplication by a power of two.
> >
> >  This change does the reverse of the conversion described above and makes
> > TARGET_LEGITIMATE_ADDRESS_P and possibly other backend code be presented 
> > with either form for indexed addresses, which complicates handling.  The 
> > ICE mentioned above specifically is caused by:
> >
> > (plus:SI (plus:SI (mult:SI (reg:SI 30 [ _10 ])
> >             (const_int 4 [0x4]))
> >         (reg/f:SI 26 [ _6 ]))
> >     (const_int 12 [0xc]))
> 
> …if you write:
> 
> -----------------------------------------------------------
> long *foo ();
> long *bar (long *ptr, long x) { return &foo ()[x + 3]; }
> -----------------------------------------------------------
> 
> then the rtl you get is:
> 
> -----------------------------------------------------------
> …
> (insn 10 9 11 2 (set (reg:SI 32)
>         (plus:SI (reg/v:SI 29 [ x ])
>             (const_int 3 [0x3]))) "/tmp/foo.c":2:47 -1
>      (nil))
> (insn 11 10 12 2 (set (reg:SI 33)
>         (ashift:SI (reg:SI 32)
>             (const_int 2 [0x2]))) "/tmp/foo.c":2:47 -1
>      (nil))
> (insn 12 11 13 2 (set (reg:SI 31)
>         (plus:SI (reg/f:SI 23 [ _1 ])
>             (reg:SI 33))) "/tmp/foo.c":2:40 -1
>      (nil))
> …
> -----------------------------------------------------------
> 
> where the address uses “ashift” rather than “mult”.  Then combine
> tries to generate the same kind of address as the one you quote above,
> but with “ashift” rather than “mult”:
> 
> -----------------------------------------------------------
> Trying 10, 11 -> 12:
>    10: r32:SI=r29:SI+0x3
>       REG_DEAD r29:SI
>    11: r33:SI=r32:SI<<0x2
>       REG_DEAD r32:SI
>    12: r31:SI=r34:SI+r33:SI
>       REG_DEAD r34:SI
>       REG_DEAD r33:SI
> Failed to match this instruction:
> (set (reg:SI 31)
>     (plus:SI (plus:SI (ashift:SI (reg/v:SI 29 [ x ])
>                 (const_int 2 [0x2]))
>             (reg:SI 34))
>         (const_int 12 [0xc])))
> -----------------------------------------------------------
> 
> So I don't see your VAX change as papering over the issue.  The above
> “ashift” form is what address calculations normally look like outside
> of a “mem”.  The point of the rtl canonicalisation rules is to make sure
> that targets don't need to support two different ways of writing the
> same thing, which in this case means not having to support
> “mult”-by-a-power-of-2 as well as “ashift” for the LEA above.
> 
> > coming from:
> >
> > (insn 58 57 59 10 (set (reg:SI 33 [ _13 ])
> >         (zero_extract:SI (mem:SI (plus:SI (plus:SI (mult:SI (reg:SI 30 [ _10
> ])
> >                             (const_int 4 [0x4]))
> >                         (reg/f:SI 26 [ _6 ]))
> >                     (const_int 12 [0xc])) [4 _6->bits[_10]+0 S4 A32])
> >             (reg:QI 56)
> >             (reg:SI 53))) 
> > ".../gcc/testsuite/gcc.c-torture/execute/20090113-2.c":64:12 490
> {*extzv_non_const}
> >      (expr_list:REG_DEAD (reg:QI 56)
> >         (expr_list:REG_DEAD (reg:SI 53)
> >             (expr_list:REG_DEAD (reg:SI 30 [ _10 ])
> >                 (expr_list:REG_DEAD (reg/f:SI 26 [ _6 ])
> >                     (nil))))))
> >
> > being converted into:
> >
> > (plus:SI (plus:SI (ashift:SI (reg:SI 30 [ _10 ])
> >             (const_int 2 [0x2]))
> >         (reg/f:SI 26 [ _6 ]))
> >     (const_int 12 [0xc]))
> >
> > which the backend currently does not recognise as a valid machine address 
> > and clearly all the fallback handling fails in this case.  It also causes 
> > indexed addressing for non-byte data (scaling is implicit in the VAX ISA) 
> > to cease being used where no ICE actually triggers, which causes a serious 
> > code quality regression from extraneous manual address calculations.
> >
> >  Given that the VAX backend currently does not expect ASHIFT in addresses 
> > and it works, this single piece in LRA must be the only one across the 
> > middle end that uses this form and all the other code must have stuck to 
> > the MULT form.  So it does not appear to me that ASHIFT form indeed used 
> > not to be considered canonical until this problematic change.
> 
> Hopefully the above combine example answers this.
> 
> >  I have looked into what other backends do that support scaled indexed 
> > addressing and x86 escaped a regression here owing to an unrelated much 
> > earlier fix: <https://gcc.gnu.org/ml/gcc-patches/2010-04/msg01170.html> 
> > for PR target/43766 (commit 90f775a9c7af) that added ASHIFT support to its 
> > TARGET_LEGITIMATE_ADDRESS_P handler, and Aarch64 presumably has always had 
> > it.
> >
> >  I have therefore made an experimental change for the VAX backend to 
> > accept ASHIFT in its TARGET_LEGITIMATE_ADDRESS_P handler and just like 
> > reverting this change it makes the ICE go away and indexed addressing to 
> > be used again.  However there are numerous other places across the VAX 
> > backend that expect addresses to be in the MULT from, including in 
> > particular expression cost calculation, and it is not clear to me if they 
> > all have to be adjusted for the possibility created by this change for 
> > addresses to come in either form.
> 
> Does the patch also help to optimise my example above?  If so,
> it sounds like a good thing for that reason alone.
> 
> >  So why do we want to use a different canonical form for addresses 
> > depending on the context they are used with?
> >
> >  It does complicate handling in the backend and my understanding has been 
> > that canonicalisation is meant to simplify handling throughout instead.  
> > And sadly the change description does not explain why it is correct to 
> > have addresses use the ASHIFT form in certain contexts and the MULT form 
> > in the remaining ones.
> 
> Yeah, canonicalisation is supposed to simplify handling.  I think the
> thing that thwarts that in this particular context is the fact that
> we have different rules for “mult”s inside addresses.  IMO that was
> a mistake, and “mult” by a power of 2 should be an “ashift”
> wherever it occurs.  But fixing that for all backends would probably
> be a huge task.
> 
> While the special “mem” case exists, we're trading complication in
> one direction for complication in another.  If code matches addresses
> that are used for both address constraints and memory constraints,
> it will need to support both the “ashift” and “mem” forms (and for
> good code quality it should have done that even before the patch).
> On the other side, rtl patterns that match address-like expressions
> can rely on “ashift” being used and do not need to support “mult”.

cf. <https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567318.html> 
(downthread from the message referred from the commit description; shall I 
point to Richard's answer perhaps?), which is why commit c605a8bf9270 
("VAX: Accept ASHIFT in address expressions") was created.

> > .../libgcc/libgcov-driver.c: In function 'gcov_do_dump':
> > .../libgcc/libgcov-driver.c:686:1: error: insn does not satisfy its
> > constraints:
> >    686 | }
> >        | ^
> > (insn 2051 2050 2052 185 (set (reg/f:SI 0 %r0 [555])
> >          (plus:SI (ashift:SI (mem/c:SI (plus:SI (reg/f:SI 13 %fp)
> >                          (const_int -28 [0xffffffffffffffe4])) [40 %sfp+-28
> > S4 A32])
> >                  (const_int 3 [0x3]))
> >              (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
> >                  (const_int 24 [0x18]))))
> > ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
> >       (nil))
> I'm guessing this insn is the result of reloading an address within a MEM into
> a REG.

 No, the address has never been in a MEM in the first place.  The original 
insns are as follows:

(insn 2049 2048 2050 166 (set (reg:SI 553)
        (plus:SI (reg/v:SI 154 [ n_ctrs ])
            (const_int 3 [0x3]))) ".../libgcc/libgcov-driver.c":172:40 201 {addsi3}
     (nil))
(insn 2050 2049 2051 166 (set (reg:SI 554)
        (ashift:SI (reg:SI 553)
            (const_int 3 [0x3]))) ".../libgcc/libgcov-driver.c":172:40 432 {ashlsi3}
     (expr_list:REG_DEAD (reg:SI 553)
        (nil)))
(insn 2051 2050 2052 166 (set (reg/f:SI 555)
        (plus:SI (reg/v/f:SI 176 [ fn_buffer ])
            (reg:SI 554))) ".../libgcc/libgcov-driver.c":172:40 201 {addsi3}
     (expr_list:REG_DEAD (reg:SI 554)
        (nil)))

and then combine merges them as follows (pretty damn good job IMO!):

(note 2049 2048 2050 166 NOTE_INSN_DELETED)
(note 2050 2049 2051 166 NOTE_INSN_DELETED)
(insn 2051 2050 2052 166 (set (reg/f:SI 555)
        (plus:SI (plus:SI (ashift:SI (reg/v:SI 154 [ n_ctrs ])
                    (const_int 3 [0x3]))
                (reg/v/f:SI 176 [ fn_buffer ]))
            (const_int 24 [0x18]))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
     (nil))

and then reload substitutes (reg/v:SI 154 [ n_ctrs ]) with the inner MEM 
as it fails to reload the pseudo and just uses its memory location.

 For the record libgcc/libgcov-driver.c has this at line 172 as at commit 
4a960d548b7d:

      fn_buffer->info.ctrs[n_ctrs].num = length;
      fn_buffer->info.ctrs[n_ctrs].values = values;

so I guess this insn calculates `fn_buffer->info.ctrs + n_ctrs'.

>   Had that address been in a canonical form I don't think this patch
> would be needed.

 I guess so.  In any case reverting commit c605a8bf9270 does make the 
compiler work (hence I marked the issue as a regression), but said commit 
is needed for LRA to work with the VAX target, and as noted with the 
original submission it also improves code generation with old reload, so 
it's not only that LRA relies on this non-canonical form.

> Am I missing something?

 Well, am *I* missing something?

  Maciej
  

On 11/4/2021 3:04 PM, Maciej W. Rozycki wrote:
> On Thu, 4 Nov 2021, Jeff Law wrote:
>
>> On 11/3/2021 7:53 AM, Maciej W. Rozycki wrote:
>>> Correct a `vax-netbsdelf' target regression ultimately caused by commit
>>> c605a8bf9270 ("VAX: Accept ASHIFT in address expressions") (needed for
>>> LRA) and as of commit 4a960d548b7d ("Avoid invalid loop transformations
>>> in jump threading registry.") causing a build error in libgcc:
>> But within a MEM the ASHIFT should have been canonicalized into a MULT by an
>> appropriate power of two according to the canonicalization rules.
>   I thought so as well, but was straigtened out by Richard:
[ ... ]
snip.

Sometimes the language we're using in email is not as crisp as it should 
be.  So just to be clear, the canonicalization I'm referring to is only 
in effect within a MEM.  It does not apply to address calculations that 
happen outside a MEM.  I think that is consistent with Richard's comments.


>>> .../libgcc/libgcov-driver.c: In function 'gcov_do_dump':
>>> .../libgcc/libgcov-driver.c:686:1: error: insn does not satisfy its
>>> constraints:
>>>     686 | }
>>>         | ^
>>> (insn 2051 2050 2052 185 (set (reg/f:SI 0 %r0 [555])
>>>           (plus:SI (ashift:SI (mem/c:SI (plus:SI (reg/f:SI 13 %fp)
>>>                           (const_int -28 [0xffffffffffffffe4])) [40 %sfp+-28
>>> S4 A32])
>>>                   (const_int 3 [0x3]))
>>>               (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
>>>                   (const_int 24 [0x18]))))
>>> ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
>>>        (nil))
>> I'm guessing this insn is the result of reloading an address within a MEM into
>> a REG.
>   No, the address has never been in a MEM in the first place.  The original
> insns are as follows:
OK.  So this isn't about canonicalization with in MEMs...
>
> (insn 2049 2048 2050 166 (set (reg:SI 553)
>          (plus:SI (reg/v:SI 154 [ n_ctrs ])
>              (const_int 3 [0x3]))) ".../libgcc/libgcov-driver.c":172:40 201 {addsi3}
>       (nil))
> (insn 2050 2049 2051 166 (set (reg:SI 554)
>          (ashift:SI (reg:SI 553)
>              (const_int 3 [0x3]))) ".../libgcc/libgcov-driver.c":172:40 432 {ashlsi3}
>       (expr_list:REG_DEAD (reg:SI 553)
>          (nil)))
> (insn 2051 2050 2052 166 (set (reg/f:SI 555)
>          (plus:SI (reg/v/f:SI 176 [ fn_buffer ])
>              (reg:SI 554))) ".../libgcc/libgcov-driver.c":172:40 201 {addsi3}
>       (expr_list:REG_DEAD (reg:SI 554)
>          (nil)))
>
> and then combine merges them as follows (pretty damn good job IMO!):
>
> (note 2049 2048 2050 166 NOTE_INSN_DELETED)
> (note 2050 2049 2051 166 NOTE_INSN_DELETED)
> (insn 2051 2050 2052 166 (set (reg/f:SI 555)
>          (plus:SI (plus:SI (ashift:SI (reg/v:SI 154 [ n_ctrs ])
>                      (const_int 3 [0x3]))
>                  (reg/v/f:SI 176 [ fn_buffer ]))
>              (const_int 24 [0x18]))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
>       (nil))
So far, so good.

>
> and then reload substitutes (reg/v:SI 154 [ n_ctrs ]) with the inner MEM
> as it fails to reload the pseudo and just uses its memory location.
OK.  So what I still don't see is why  we would need to re-recognize.   
You're changing code that I thought was only applicable when we were 
reloading an address inside a MEM and if we're inside a MEM, then we 
shouldn't be seeing an ASHIFT.   We're replacing the argument of the ASHIFT.


So, overall, I'm still confused as to why the patch has any effect at all.

jeff
  

On Thu, 4 Nov 2021, Jeff Law wrote:

> Sometimes the language we're using in email is not as crisp as it should be.  So
> just to be clear, the canonicalization I'm referring to is only in effect within
> a MEM.  It does not apply to address calculations that happen outside a MEM.  I
> think that is consistent with Richard's comments.

 Ah, OK then.

> > and then reload substitutes (reg/v:SI 154 [ n_ctrs ]) with the inner MEM
> > as it fails to reload the pseudo and just uses its memory location.
> OK.  So what I still don't see is why  we would need to re-recognize.   You're
> changing code that I thought was only applicable when we were reloading an
> address inside a MEM and if we're inside a MEM, then we shouldn't be seeing an
> ASHIFT.   We're replacing the argument of the ASHIFT.

 Well, the context of this code (around and including hunk #1) is:

      else if (insn_extra_address_constraint
	       (lookup_constraint (constraints[i])))
	{
	  address_operand_reloaded[i]
	    = find_reloads_address (recog_data.operand_mode[i], (rtx*) 0,
				    recog_data.operand[i],
				    recog_data.operand_loc[i],
				    i, operand_type[i], ind_levels, insn);

	  /* If we now have a simple operand where we used to have a
	     PLUS or MULT, re-recognize and try again.  */
	  if ((OBJECT_P (*recog_data.operand_loc[i])
	       || GET_CODE (*recog_data.operand_loc[i]) == SUBREG)
	      && (GET_CODE (recog_data.operand[i]) == MULT
		  || GET_CODE (recog_data.operand[i]) == PLUS))
	    {
	      INSN_CODE (insn) = -1;
	      retval = find_reloads (insn, replace, ind_levels, live_known,
				     reload_reg_p);
	      return retval;
	    }

so the body of the conditional is specifically executed for an address and 
not a MEM; in this particular case matched with the plain "p" constraint.  

 MEMs are handled with the next conditional right below.

> So, overall, I'm still confused as to why the patch has any effect at all.

 Does the explanation above clear your confusion?

  Maciej
  

On Wed, 3 Nov 2021, Maciej W. Rozycki wrote:
> Correct a `vax-netbsdelf' target regression ultimately caused by commit
> c605a8bf9270 ("VAX: Accept ASHIFT in address expressions") (needed for
> LRA) and as of commit 4a960d548b7d ("Avoid invalid loop transformations
> in jump threading registry.") causing a build error in libgcc:
>
> .../libgcc/libgcov-driver.c: In function 'gcov_do_dump':
> .../libgcc/libgcov-driver.c:686:1: error: insn does not satisfy its constraints:
>   686 | }
>       | ^
> (insn 2051 2050 2052 185 (set (reg/f:SI 0 %r0 [555])
>         (plus:SI (ashift:SI (mem/c:SI (plus:SI (reg/f:SI 13 %fp)
>                         (const_int -28 [0xffffffffffffffe4])) [40 %sfp+-28 S4 A32])
>                 (const_int 3 [0x3]))
>             (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
>                 (const_int 24 [0x18])))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
>      (nil))
> during RTL pass: postreload

[...]

>  As noted in the commit description this has been regression-tested with
> commit 4a960d548b7d^.  I'm running regression-testing with GCC 11 right
> now as well and expect results by the end of week.
>
>  I was trying to chase another target I could use to regression-test this
> with that does do scaled indexed addressing while still using old reload.
> The i386 port would be a good candidate, but it has switched to LRA long
> ago with no option to use old reload, and I think there would be little
> point in adding one just for the sake of such verification.  Do we have
> any other port actually that could be affected by this change?

That'd be cris-elf.

Your proposed patch reminded me of 6cb68940dcf9; giving reload a
reload-specific insn_and_split pattern to play with, matching
"mult" outside of a mem.  I *guess* that's the CRIS-specific
replacement to c605a8bf9270.

brgds, H-P
  

On Fri, 5 Nov 2021, Hans-Peter Nilsson wrote:

> >  I was trying to chase another target I could use to regression-test this
> > with that does do scaled indexed addressing while still using old reload.
> > The i386 port would be a good candidate, but it has switched to LRA long
> > ago with no option to use old reload, and I think there would be little
> > point in adding one just for the sake of such verification.  Do we have
> > any other port actually that could be affected by this change?
> 
> That'd be cris-elf.

 Good to know, thanks!

 How do I run regression-testing with this target however?  I can see QEMU
support upstream, even for user-mode Linux, which would be the easiest to 
run (sadly toolchain support for CRIS/Linux was removed a while ago as was 
the Linux kernel port; at one point I even considered getting myself a 
CRIS development board as an alternative RISC platform that would Linux, 
but concluded that it was too expensive for the features it offered), but 
for a bare metal environment both a C library (newlib?) and then a 
specific board support package is required.

 Or may I ask you to put this patch through testing with your environment?

> Your proposed patch reminded me of 6cb68940dcf9; giving reload a
> reload-specific insn_and_split pattern to play with, matching
> "mult" outside of a mem.  I *guess* that's the CRIS-specific
> replacement to c605a8bf9270.

 Possibly, except for the missing reload bits making it incomplete.

  Maciej
  

On Wed, 3 Nov 2021, Maciej W. Rozycki wrote:

>  I have not yet tracked down which change after commit c605a8bf9270 made 
> regressions appear in the test suites, however clearly the commit wasn't 
> as complete a change as it should have been.  I'll see if I can find it 
> and will mention it in the final commit description if there is anything 
> useful in that.

 So regrettably it's my original commit c605a8bf9270 that caused these 
regressions.  I don't know how it happened that it slipped through in 
testing; I may have had the change left in the tree that enabled LRA and 
did not notice it.  Sorry about that, umm.

>  As noted in the commit description this has been regression-tested with 
> commit 4a960d548b7d^.  I'm running regression-testing with GCC 11 right 
> now as well and expect results by the end of week.

 For the record the change does fix numerous regressions in GCC 11 too, 
e.g.:

		=== gcc Summary ===

# of expected passes		122244
# of unexpected failures	1852
# of expected passes		122428
# of unexpected failures	1484
# of unexpected successes	5
# of expected failures		721
# of unresolved testcases	10

So either this change, if deemed safe enough, will have to go into GCC 11, 
or, if we're going to keep old reload yet for GCC 12, then the best course 
of action for GCC 11 might be reverting commit c605a8bf9270, although at 
the cost of regressing code quality a bit where it does build.

 Suggestions?

  Maciej
  

On Sun, 7 Nov 2021, Maciej W. Rozycki wrote:
> On Fri, 5 Nov 2021, Hans-Peter Nilsson wrote:
>
> > >  I was trying to chase another target I could use to regression-test this
> > > with that does do scaled indexed addressing while still using old reload.
> > > The i386 port would be a good candidate, but it has switched to LRA long
> > > ago with no option to use old reload, and I think there would be little
> > > point in adding one just for the sake of such verification.  Do we have
> > > any other port actually that could be affected by this change?
> >
> > That'd be cris-elf.
>
>  Good to know, thanks!
>
>  How do I run regression-testing with this target however?  I can see QEMU
> support upstream, even for user-mode Linux, which would be the easiest to
> run (sadly toolchain support for CRIS/Linux was removed a while ago as was
> the Linux kernel port; at one point I even considered getting myself a
> CRIS development board as an alternative RISC platform that would Linux,
> but concluded that it was too expensive for the features it offered), but
> for a bare metal environment both a C library (newlib?) and then a
> specific board support package is required.

Classic "bare-metal" whatever-elf testing should not be a
stranger: sim and binutils support are in place in the official
binutils+gdb git, as is newlib in that git and since many
dejagnu releases a cris-sim.exp baseboard file.  Just build and
install binutils and sim for cris-elf (can probable be done at
the same time/same builds from a binutils-and-gdb checkout, but
separate builds are sometimes necessary) then build and test gcc
from a combined-source-tree containing newlib and gcc.
(Instructions for combining trees may be salvaged from the
rottening https://gcc.gnu.org/simtest-howto.html but actually I
roll tarballs and untar gcc over an (untarred) newlib tree.)

I don't have a baseboard file for QEMU, sorry.

>  Or may I ask you to put this patch through testing with your environment?

Where's the fun in that? :)
(I thought you'd use 6cb68940dcf9 and do the same for VAX.)

> > Your proposed patch reminded me of 6cb68940dcf9; giving reload a
> > reload-specific insn_and_split pattern to play with, matching
> > "mult" outside of a mem.  I *guess* that's the CRIS-specific
> > replacement to c605a8bf9270.
>
>  Possibly, except for the missing reload bits making it incomplete.

No, my thinking was that it wouldn't be needed.  But, I didn't
have a close look and maybe the problem isn't exactly the same
or VAX has additional caveats.  Also, that reload-pacifying
pattern *is* a target-specific workaround for a reload bug, but
a risk-free one for other targets.

brgds, H-P
PS. I'll fire up a round with that patch "tomorrow".  Film at 11.
  

On Sun, 7 Nov 2021, Hans-Peter Nilsson wrote:

> >  How do I run regression-testing with this target however?  I can see QEMU
> > support upstream, even for user-mode Linux, which would be the easiest to
> > run (sadly toolchain support for CRIS/Linux was removed a while ago as was
> > the Linux kernel port; at one point I even considered getting myself a
> > CRIS development board as an alternative RISC platform that would Linux,
> > but concluded that it was too expensive for the features it offered), but
> > for a bare metal environment both a C library (newlib?) and then a
> > specific board support package is required.
> 
> Classic "bare-metal" whatever-elf testing should not be a
> stranger: sim and binutils support are in place in the official
> binutils+gdb git, as is newlib in that git and since many
> dejagnu releases a cris-sim.exp baseboard file.  Just build and
> install binutils and sim for cris-elf (can probable be done at
> the same time/same builds from a binutils-and-gdb checkout, but
> separate builds are sometimes necessary) then build and test gcc
> from a combined-source-tree containing newlib and gcc.
> (Instructions for combining trees may be salvaged from the
> rottening https://gcc.gnu.org/simtest-howto.html but actually I
> roll tarballs and untar gcc over an (untarred) newlib tree.)

 Thanks, I'll give it a try.  I don't use GNUsim-based configurations very 
often, so I'm not even used to thinking they exist.  It might be good to 
have a template build configuration then.

> >  Or may I ask you to put this patch through testing with your environment?
> 
> Where's the fun in that? :)
> (I thought you'd use 6cb68940dcf9 and do the same for VAX.)

 I could, easily, but being confined to gcc/config/cris I don't expect it 
to be included in the build let alone trigger anything.

> > > Your proposed patch reminded me of 6cb68940dcf9; giving reload a
> > > reload-specific insn_and_split pattern to play with, matching
> > > "mult" outside of a mem.  I *guess* that's the CRIS-specific
> > > replacement to c605a8bf9270.
> >
> >  Possibly, except for the missing reload bits making it incomplete.
> 
> No, my thinking was that it wouldn't be needed.  But, I didn't
> have a close look and maybe the problem isn't exactly the same
> or VAX has additional caveats.  Also, that reload-pacifying
> pattern *is* a target-specific workaround for a reload bug, but
> a risk-free one for other targets.

 Right.

> brgds, H-P
> PS. I'll fire up a round with that patch "tomorrow".  Film at 11.

 Great, thanks!  I'll build and test your configuration anyway.  Though I 
can see that CRIS has LRA wired to off right now, which means there'll be 
little interference likely from my upcoming work with the VAX port as I'm 
going to focus on making LRA better now rather than poking at old reload 
unless something else as grave as this issue pops up.

  Maciej
  

> From: "Maciej W. Rozycki" <macro@embecosm.com>
> Date: Wed, 3 Nov 2021 14:53:58 +0100

> 	gcc/
> 	PR middle-end/103059
> 	* reload.c (find_reloads_address_1): Also accept the ASHIFT form 
> 	of indexed addressing.
> 	(find_reloads): Adjust accordingly.

> ---
>  gcc/reload.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> gcc-find-reloads-address-ashift.diff
> Index: gcc/gcc/reload.c
> ===================================================================
> --- gcc.orig/gcc/reload.c
> +++ gcc/gcc/reload.c
> @@ -2846,10 +2846,11 @@ find_reloads (rtx_insn *insn, int replac
>  				    i, operand_type[i], ind_levels, insn);
>  
>  	  /* If we now have a simple operand where we used to have a
> -	     PLUS or MULT, re-recognize and try again.  */
> +	     PLUS or MULT or ASHIFT, re-recognize and try again.  */
>  	  if ((OBJECT_P (*recog_data.operand_loc[i])
>  	       || GET_CODE (*recog_data.operand_loc[i]) == SUBREG)
>  	      && (GET_CODE (recog_data.operand[i]) == MULT
> +		  || GET_CODE (recog_data.operand[i]) == ASHIFT
>  		  || GET_CODE (recog_data.operand[i]) == PLUS))
>  	    {
>  	      INSN_CODE (insn) = -1;
> @@ -5562,7 +5563,8 @@ find_reloads_address_1 (machine_mode mod
>  	    return 1;
>  	  }
>  
> -	if (code0 == MULT || code0 == SIGN_EXTEND || code0 == TRUNCATE
> +	if (code0 == MULT || code0 == ASHIFT
> +	    || code0 == SIGN_EXTEND || code0 == TRUNCATE
>  	    || code0 == ZERO_EXTEND || code1 == MEM)
>  	  {
>  	    find_reloads_address_1 (mode, as, orig_op0, 1, PLUS, SCRATCH,
> @@ -5573,7 +5575,8 @@ find_reloads_address_1 (machine_mode mod
>  				    insn);
>  	  }
>  
> -	else if (code1 == MULT || code1 == SIGN_EXTEND || code1 == TRUNCATE
> +	else if (code1 == MULT || code1 == ASHIFT
> +		 || code1 == SIGN_EXTEND || code1 == TRUNCATE
>  		 || code1 == ZERO_EXTEND || code0 == MEM)
>  	  {
>  	    find_reloads_address_1 (mode, as, orig_op0, 0, PLUS, code1,
> 

I regression-tested this patch for cris-elf at
r12-4987-g14e355df3053.  No regressions compared to
r12-4987-g14e355df3053.  (JFTR, that's at regress-11,
compared to T0=2007-01-05-16:47:21).

brgds, H-P
  

> From: "Maciej W. Rozycki" <macro@embecosm.com>
> Date: Wed, 3 Nov 2021 14:53:58 +0100

> 	gcc/
> 	PR middle-end/103059
> 	* reload.c (find_reloads_address_1): Also accept the ASHIFT form 
> 	of indexed addressing.
> 	(find_reloads): Adjust accordingly.

> ---
>  gcc/reload.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> gcc-find-reloads-address-ashift.diff
> Index: gcc/gcc/reload.c
> ===================================================================
> --- gcc.orig/gcc/reload.c
> +++ gcc/gcc/reload.c
> @@ -2846,10 +2846,11 @@ find_reloads (rtx_insn *insn, int replac
>  				    i, operand_type[i], ind_levels, insn);
>  
>  	  /* If we now have a simple operand where we used to have a
> -	     PLUS or MULT, re-recognize and try again.  */
> +	     PLUS or MULT or ASHIFT, re-recognize and try again.  */
>  	  if ((OBJECT_P (*recog_data.operand_loc[i])
>  	       || GET_CODE (*recog_data.operand_loc[i]) == SUBREG)
>  	      && (GET_CODE (recog_data.operand[i]) == MULT
> +		  || GET_CODE (recog_data.operand[i]) == ASHIFT
>  		  || GET_CODE (recog_data.operand[i]) == PLUS))
>  	    {
>  	      INSN_CODE (insn) = -1;
> @@ -5562,7 +5563,8 @@ find_reloads_address_1 (machine_mode mod
>  	    return 1;
>  	  }
>  
> -	if (code0 == MULT || code0 == SIGN_EXTEND || code0 == TRUNCATE
> +	if (code0 == MULT || code0 == ASHIFT
> +	    || code0 == SIGN_EXTEND || code0 == TRUNCATE
>  	    || code0 == ZERO_EXTEND || code1 == MEM)
>  	  {
>  	    find_reloads_address_1 (mode, as, orig_op0, 1, PLUS, SCRATCH,
> @@ -5573,7 +5575,8 @@ find_reloads_address_1 (machine_mode mod
>  				    insn);
>  	  }
>  
> -	else if (code1 == MULT || code1 == SIGN_EXTEND || code1 == TRUNCATE
> +	else if (code1 == MULT || code1 == ASHIFT
> +		 || code1 == SIGN_EXTEND || code1 == TRUNCATE
>  		 || code1 == ZERO_EXTEND || code0 == MEM)
>  	  {
>  	    find_reloads_address_1 (mode, as, orig_op0, 0, PLUS, code1,
> 

I regression-tested this patch for cris-elf at
r12-4987-g14e355df3053.  No regressions compared to
r12-4987-g14e355df3053.  (JFTR, that's at regress-11,
compared to T0=2007-01-05-16:47:21).

brgds, H-P
  

On 11/4/2021 6:18 PM, Maciej W. Rozycki wrote:
> On Thu, 4 Nov 2021, Jeff Law wrote:
>
>> Sometimes the language we're using in email is not as crisp as it should be.  So
>> just to be clear, the canonicalization I'm referring to is only in effect within
>> a MEM.  It does not apply to address calculations that happen outside a MEM.  I
>> think that is consistent with Richard's comments.
>   Ah, OK then.
>
>>> and then reload substitutes (reg/v:SI 154 [ n_ctrs ]) with the inner MEM
>>> as it fails to reload the pseudo and just uses its memory location.
>> OK.  So what I still don't see is why  we would need to re-recognize.   You're
>> changing code that I thought was only applicable when we were reloading an
>> address inside a MEM and if we're inside a MEM, then we shouldn't be seeing an
>> ASHIFT.   We're replacing the argument of the ASHIFT.
>   Well, the context of this code (around and including hunk #1) is:
>
>        else if (insn_extra_address_constraint
> 	       (lookup_constraint (constraints[i])))
> 	{
> 	  address_operand_reloaded[i]
> 	    = find_reloads_address (recog_data.operand_mode[i], (rtx*) 0,
> 				    recog_data.operand[i],
> 				    recog_data.operand_loc[i],
> 				    i, operand_type[i], ind_levels, insn);
>
> 	  /* If we now have a simple operand where we used to have a
> 	     PLUS or MULT, re-recognize and try again.  */
> 	  if ((OBJECT_P (*recog_data.operand_loc[i])
> 	       || GET_CODE (*recog_data.operand_loc[i]) == SUBREG)
> 	      && (GET_CODE (recog_data.operand[i]) == MULT
> 		  || GET_CODE (recog_data.operand[i]) == PLUS))
> 	    {
> 	      INSN_CODE (insn) = -1;
> 	      retval = find_reloads (insn, replace, ind_levels, live_known,
> 				     reload_reg_p);
> 	      return retval;
> 	    }
>
> so the body of the conditional is specifically executed for an address and
> not a MEM; in this particular case matched with the plain "p" constraint.
>
>   MEMs are handled with the next conditional right below.
Ah!  Thanks for the clarification.  We're digging deep into history 
here.  I always thought this code was re-recognizing inside a MEM, but 
as you note, it's actually handling stuff outside the MEM, such as  a 
'p' constraint, which is an address, but being outside a MEMS means its 
not subject to the mult-by-power-of-2 canonicalization.

So I think the first hunk is fine.  There's two others that twiddle 
find_reloads_address_1, which I think can only be reached from 
find_reloads_address.  The comment at the front would indicate it's only 
called where AD is inside a MEM.

Are we getting into find_reloads_address_1 in any case where the RTL is 
not an address inside a MEM?

jeff
  

On Sun, 7 Nov 2021, Maciej W. Rozycki wrote:

>  For the record the change does fix numerous regressions in GCC 11 too, 
> e.g.:
> 
> 		=== gcc Summary ===
> 
> # of expected passes		122244
> # of unexpected failures	1852
> # of expected passes		122428
> # of unexpected failures	1484
> # of unexpected successes	5
> # of expected failures		721
> # of unresolved testcases	10

 Umm, that was supposed to be:

 		=== gcc Summary ===
 
-# of expected passes		122244
-# of unexpected failures	1852
+# of expected passes		122428
+# of unexpected failures	1484
 # of unexpected successes	5
 # of expected failures		721
 # of unresolved testcases	10

> So either this change, if deemed safe enough, will have to go into GCC 11, 
> or, if we're going to keep old reload yet for GCC 12, then the best course 
> of action for GCC 11 might be reverting commit c605a8bf9270, although at 
> the cost of regressing code quality a bit where it does build.

 Verification of the revert of commit c605a8bf9270 with GCC 11 shows a 
pair of regressions in libstdc++-v3 though:

-FAIL: 26_numerics/random/ranlux48.cc execution test
-FAIL: 26_numerics/random/ranlux48_base.cc execution test
+FAIL: 26_numerics/random/ranlux48.cc (test for excess errors)
+FAIL: 26_numerics/random/ranlux48_base.cc (test for excess errors)

and the excess errors are:

during RTL pass: postreload
.../libstdc++-v3/testsuite/26_numerics/random/ranlux48.cc: In function 'void test01()':
.../libstdc++-v3/testsuite/26_numerics/random/ranlux48.cc:36: internal compiler error: in cselib_invalidate_regno, at cselib.c:2422

and:

during RTL pass: postreload
.../libstdc++-v3/testsuite/26_numerics/random/ranlux48_base.cc: In function 'void test01()':
.../libstdc++-v3/testsuite/26_numerics/random/ranlux48_base.cc:36: internal compiler error: in cselib_invalidate_regno, at cselib.c:2422

respectively (we have a pair of test cases that fail anyway, but an ICE is 
I think an escalation of a failure in comparison to an execution failure; 
I haven't debugged what the causes of the respective execution failures 
are or what the exact cause of the ICEs is at this point).

 No changes elsewhere throughout all the testsuites.

  Maciej
  

On Mon, 8 Nov 2021, Hans-Peter Nilsson wrote:

> I regression-tested this patch for cris-elf at
> r12-4987-g14e355df3053.  No regressions compared to
> r12-4987-g14e355df3053.  (JFTR, that's at regress-11,
> compared to T0=2007-01-05-16:47:21).

 Great, thanks!

  Maciej
  

On Mon, 8 Nov 2021, Jeff Law wrote:

> >   Well, the context of this code (around and including hunk #1) is:
> > 
> >        else if (insn_extra_address_constraint
> > 	       (lookup_constraint (constraints[i])))
> > 	{
> > 	  address_operand_reloaded[i]
> > 	    = find_reloads_address (recog_data.operand_mode[i], (rtx*) 0,
> > 				    recog_data.operand[i],
> > 				    recog_data.operand_loc[i],
> > 				    i, operand_type[i], ind_levels, insn);
> > 
> > 	  /* If we now have a simple operand where we used to have a
> > 	     PLUS or MULT, re-recognize and try again.  */
> > 	  if ((OBJECT_P (*recog_data.operand_loc[i])
> > 	       || GET_CODE (*recog_data.operand_loc[i]) == SUBREG)
> > 	      && (GET_CODE (recog_data.operand[i]) == MULT
> > 		  || GET_CODE (recog_data.operand[i]) == PLUS))
> > 	    {
> > 	      INSN_CODE (insn) = -1;
> > 	      retval = find_reloads (insn, replace, ind_levels, live_known,
> > 				     reload_reg_p);
> > 	      return retval;
> > 	    }
> > 
> > so the body of the conditional is specifically executed for an address and
> > not a MEM; in this particular case matched with the plain "p" constraint.
> > 
> >   MEMs are handled with the next conditional right below.
> Ah!  Thanks for the clarification.  We're digging deep into history here.  I
> always thought this code was re-recognizing inside a MEM, but as you note, it's
> actually handling stuff outside the MEM, such as  a 'p' constraint, which is an
> address, but being outside a MEMS means its not subject to the
> mult-by-power-of-2 canonicalization.
> 
> So I think the first hunk is fine.  There's two others that twiddle
> find_reloads_address_1, which I think can only be reached from
> find_reloads_address.  The comment at the front would indicate it's only
> called where AD is inside a MEM.

 It's actually hunk #2 that fixes this specific ICE.  The other two are 
just a consequence: #3 just being a commutative variant of the same case 
and #1 from observing that the rtx may now have changed if an ASHIFT too.

> Are we getting into find_reloads_address_1 in any case where the RTL is not an
> address inside a MEM?

 I've had a GDB session left open with the problematic source, so it was 
merely a case of a rerun and grabbing some data.  So with a breakpoint set 
at reload.c:5565, conditionalised on (code0 == ASHIFT || code1 == ASHIFT), 
we get exactly this, as with my change description:

Breakpoint 52, find_reloads_address_1 (mode=E_DImode, as=0 '\000', x=0x7fffedbaf7b0, context=0, outer_code=MEM, index_code=SCRATCH, loc=0x7ffff61a82f0, opnum=1, type=RELOAD_FOR_INPUT, ind_levels=1, insn=0x7fffefc1c9c0) at .../gcc/reload.c:5565
5565		if (code0 == MULT || code0 == SIGN_EXTEND || code0 == TRUNCATE
(gdb) print code0
$12958 = ASHIFT
(gdb) print code1
$12959 = PLUS
(gdb) print outer_code
$12960 = MEM
(gdb) pr insn
(insn 2051 2050 2052 180 (set (reg/f:SI 0 %r0 [555])
        (plus:SI (ashift:SI (reg/v:SI 154 [ n_ctrs ])
                (const_int 3 [0x3]))
            (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
                (const_int 24 [0x18])))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
     (nil))
(gdb) pr x
(plus:SI (ashift:SI (reg/v:SI 154 [ n_ctrs ])
        (const_int 3 [0x3]))
    (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
        (const_int 24 [0x18])))
(gdb) bt
#0  find_reloads_address_1 (mode=E_DImode, as=0 '\000', x=0x7fffedbaf7b0, context=0, outer_code=MEM, index_code=SCRATCH, loc=0x7ffff61a82f0, opnum=1, type=RELOAD_FOR_INPUT, ind_levels=1, insn=0x7fffefc1c9c0) at .../gcc/reload.c:5565
#1  0x00000000111ecd18 in find_reloads_address (mode=E_DImode, memrefloc=0x0, ad=0x7fffedbaf7b0, loc=0x7ffff61a82f0, opnum=1, type=RELOAD_FOR_INPUT, ind_levels=1, insn=0x7fffefc1c9c0) at .../gcc/reload.c:5264
#2  0x00000000111e2fbc in find_reloads (insn=0x7fffefc1c9c0, replace=1, ind_levels=1, live_known=1, reload_reg_p=0x12ec7770 <spill_reg_order>) at .../gcc/reload.c:2843
#3  0x00000000112060f4 in reload_as_needed (live_known=1) at .../gcc/reload1.c:4522
#4  0x00000000111f9008 in reload (first=0x7ffff5dd3c28, global=1) at .../gcc/reload1.c:1047
#5  0x0000000010f1458c in do_reload () at .../gcc/ira.c:5944
#6  0x0000000010f14d54 in (anonymous namespace)::pass_reload::execute (this=0x12f21d20) at .../gcc/ira.c:6118
#7  0x000000001112472c in execute_one_pass (pass=0x12f21d20) at .../gcc/passes.c:2567
#8  0x0000000011124bc4 in execute_pass_list_1 (pass=0x12f21d20) at .../gcc/passes.c:2656
#9  0x0000000011124c0c in execute_pass_list_1 (pass=0x12f20b80) at .../gcc/passes.c:2657
#10 0x0000000011124cac in execute_pass_list (fn=0x7ffff5dc4b00, pass=0x12f1c900) at .../gcc/passes.c:2667
#11 0x00000000109b64f4 in cgraph_node::expand (this=0x7ffff5d65a50) at .../gcc/cgraphunit.c:1828
#12 0x00000000109b6eac in expand_all_functions () at .../gcc/cgraphunit.c:1992
#13 0x00000000109b7eb8 in symbol_table::compile (this=0x7ffff5c40000) at .../gcc/cgraphunit.c:2356
#14 0x00000000109b8638 in symbol_table::finalize_compilation_unit (this=0x7ffff5c40000) at .../gcc/cgraphunit.c:2537
#15 0x00000000112c4418 in compile_file () at .../gcc/toplev.c:477
#16 0x00000000112c8f60 in do_compile (no_backend=false) at .../gcc/toplev.c:2154
#17 0x00000000112c95d4 in toplev::main (this=0x7fffffffe944, argc=76, argv=0x7fffffffed78) at .../gcc/toplev.c:2306
#18 0x000000001245a7b8 in main (argc=76, argv=0x7fffffffed78) at .../gcc/main.c:39
(gdb) 

-- so `find_reloads_address' is called from reload.c:2843, which is the 
call site in code quoted at the top, for an address associated with the 
`p' constraint, and then it goes down to `find_reloads_address_1', which 
cannot recognise the rtx and therefore leaves it unchanged.

 Here OUTER_CODE is indeed MEM, but it's merely hardcoded by the caller 
at reload.c:5264 irrespective of actual insn/rtx:

  return find_reloads_address_1 (mode, as, ad, 0, MEM, SCRATCH, loc,
				 opnum, type, ind_levels, insn);

(I note that `find_reloads_address' does that in several places throughout 
and I haven't investigated how legitimate it is, but my guts feeling is at 
least in the case concerned it's merely a placeholder, because for a plain 
address reference it would have to be nil really.)

 Let me know if it clears your concerns and whether there's anything else 
you want me to retrieve from that GDB session.

  Maciej
  

On Mon, 8 Nov 2021, Maciej W. Rozycki wrote:
> On Sun, 7 Nov 2021, Hans-Peter Nilsson wrote:
> > (I thought you'd use 6cb68940dcf9 and do the same for VAX.)
>
>  I could, easily, but being confined to gcc/config/cris I don't expect it
> to be included in the build let alone trigger anything.

There was some level of misunderstanding here, but even looking
closer, my suggestion wouldn't help.  Still, I'll be more
verbose:

I don't think you got me here.  I mean, do as in 6cb68940dcf9
and for VAX create a define_insn_and_split, recognized at reload
time, where you recognize the MULT form and translate that into
the ASHIFT form, to help reload.

But looking closer, the situation requires a non-cc0-clobbering
insn, but AFAICT VAX only has MOVA and it clobbers
VAX_PSL_REGNUM.  Nevermind...

brgds, H-P
  

On 11/10/2021 9:41 AM, Maciej W. Rozycki wrote:
>   It's actually hunk #2 that fixes this specific ICE.  The other two are
> just a consequence: #3 just being a commutative variant of the same case
> and #1 from observing that the rtx may now have changed if an ASHIFT too.
>
>> Are we getting into find_reloads_address_1 in any case where the RTL is not an
>> address inside a MEM?
>   I've had a GDB session left open with the problematic source, so it was
> merely a case of a rerun and grabbing some data.  So with a breakpoint set
> at reload.c:5565, conditionalised on (code0 == ASHIFT || code1 == ASHIFT),
> we get exactly this, as with my change description:
>
> Breakpoint 52, find_reloads_address_1 (mode=E_DImode, as=0 '\000', x=0x7fffedbaf7b0, context=0, outer_code=MEM, index_code=SCRATCH, loc=0x7ffff61a82f0, opnum=1, type=RELOAD_FOR_INPUT, ind_levels=1, insn=0x7fffefc1c9c0) at .../gcc/reload.c:5565
> 5565		if (code0 == MULT || code0 == SIGN_EXTEND || code0 == TRUNCATE
> (gdb) print code0
> $12958 = ASHIFT
> (gdb) print code1
> $12959 = PLUS
> (gdb) print outer_code
> $12960 = MEM
> (gdb) pr insn
> (insn 2051 2050 2052 180 (set (reg/f:SI 0 %r0 [555])
>          (plus:SI (ashift:SI (reg/v:SI 154 [ n_ctrs ])
>                  (const_int 3 [0x3]))
>              (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
>                  (const_int 24 [0x18])))) ".../libgcc/libgcov-driver.c":172:40 614 {movaddrdi}
>       (nil))
> (gdb) pr x
> (plus:SI (ashift:SI (reg/v:SI 154 [ n_ctrs ])
>          (const_int 3 [0x3]))
>      (plus:SI (reg/v/f:SI 9 %r9 [orig:176 fn_buffer ] [176])
>          (const_int 24 [0x18])))
> (gdb) bt
> #0  find_reloads_address_1 (mode=E_DImode, as=0 '\000', x=0x7fffedbaf7b0, context=0, outer_code=MEM, index_code=SCRATCH, loc=0x7ffff61a82f0, opnum=1, type=RELOAD_FOR_INPUT, ind_levels=1, insn=0x7fffefc1c9c0) at .../gcc/reload.c:5565
> #1  0x00000000111ecd18 in find_reloads_address (mode=E_DImode, memrefloc=0x0, ad=0x7fffedbaf7b0, loc=0x7ffff61a82f0, opnum=1, type=RELOAD_FOR_INPUT, ind_levels=1, insn=0x7fffefc1c9c0) at .../gcc/reload.c:5264
> #2  0x00000000111e2fbc in find_reloads (insn=0x7fffefc1c9c0, replace=1, ind_levels=1, live_known=1, reload_reg_p=0x12ec7770 <spill_reg_order>) at .../gcc/reload.c:2843
> #3  0x00000000112060f4 in reload_as_needed (live_known=1) at .../gcc/reload1.c:4522
> #4  0x00000000111f9008 in reload (first=0x7ffff5dd3c28, global=1) at .../gcc/reload1.c:1047
> #5  0x0000000010f1458c in do_reload () at .../gcc/ira.c:5944
> #6  0x0000000010f14d54 in (anonymous namespace)::pass_reload::execute (this=0x12f21d20) at .../gcc/ira.c:6118
> #7  0x000000001112472c in execute_one_pass (pass=0x12f21d20) at .../gcc/passes.c:2567
> #8  0x0000000011124bc4 in execute_pass_list_1 (pass=0x12f21d20) at .../gcc/passes.c:2656
> #9  0x0000000011124c0c in execute_pass_list_1 (pass=0x12f20b80) at .../gcc/passes.c:2657
> #10 0x0000000011124cac in execute_pass_list (fn=0x7ffff5dc4b00, pass=0x12f1c900) at .../gcc/passes.c:2667
> #11 0x00000000109b64f4 in cgraph_node::expand (this=0x7ffff5d65a50) at .../gcc/cgraphunit.c:1828
> #12 0x00000000109b6eac in expand_all_functions () at .../gcc/cgraphunit.c:1992
> #13 0x00000000109b7eb8 in symbol_table::compile (this=0x7ffff5c40000) at .../gcc/cgraphunit.c:2356
> #14 0x00000000109b8638 in symbol_table::finalize_compilation_unit (this=0x7ffff5c40000) at .../gcc/cgraphunit.c:2537
> #15 0x00000000112c4418 in compile_file () at .../gcc/toplev.c:477
> #16 0x00000000112c8f60 in do_compile (no_backend=false) at .../gcc/toplev.c:2154
> #17 0x00000000112c95d4 in toplev::main (this=0x7fffffffe944, argc=76, argv=0x7fffffffed78) at .../gcc/toplev.c:2306
> #18 0x000000001245a7b8 in main (argc=76, argv=0x7fffffffed78) at .../gcc/main.c:39
> (gdb)
>
> -- so `find_reloads_address' is called from reload.c:2843, which is the
> call site in code quoted at the top, for an address associated with the
> `p' constraint, and then it goes down to `find_reloads_address_1', which
> cannot recognise the rtx and therefore leaves it unchanged.
>
>   Here OUTER_CODE is indeed MEM, but it's merely hardcoded by the caller
> at reload.c:5264 irrespective of actual insn/rtx:
>
>    return find_reloads_address_1 (mode, as, ad, 0, MEM, SCRATCH, loc,
> 				 opnum, type, ind_levels, insn);
>
> (I note that `find_reloads_address' does that in several places throughout
> and I haven't investigated how legitimate it is, but my guts feeling is at
> least in the case concerned it's merely a placeholder, because for a plain
> address reference it would have to be nil really.)
>
>   Let me know if it clears your concerns and whether there's anything else
> you want me to retrieve from that GDB session.
Thanks for the clarifications.  I never would have guessed that we could 
get into that code in the way you've described, but being reload nothing 
should be terribly surprising.

All my concerns have been addressed.  This is fine for the trunk. Thanks 
for your patience & explanations.

Jeff
  

On Tue, 23 Nov 2021, Jeff Law wrote:

> >   Let me know if it clears your concerns and whether there's anything else
> > you want me to retrieve from that GDB session.
> Thanks for the clarifications.  I never would have guessed that we could get
> into that code in the way you've described, but being reload nothing should be
> terribly surprising.
> 
> All my concerns have been addressed.  This is fine for the trunk. Thanks for
> your patience & explanations.

 Thank you for your review, I have committed this change to trunk now.

 Richard, OK for GCC 11 (correcting a VAX target regression caused by
commit 204213fdf23d)?

  Maciej
  

On Wed, 24 Nov 2021, Maciej W. Rozycki wrote:

> On Tue, 23 Nov 2021, Jeff Law wrote:
> 
> > >   Let me know if it clears your concerns and whether there's anything else
> > > you want me to retrieve from that GDB session.
> > Thanks for the clarifications.  I never would have guessed that we could get
> > into that code in the way you've described, but being reload nothing should be
> > terribly surprising.
> > 
> > All my concerns have been addressed.  This is fine for the trunk. Thanks for
> > your patience & explanations.
> 
>  Thank you for your review, I have committed this change to trunk now.
> 
>  Richard, OK for GCC 11 (correcting a VAX target regression caused by
> commit 204213fdf23d)?

OK.

Richard.
  

On Wed, 24 Nov 2021, Richard Biener wrote:

> > > All my concerns have been addressed.  This is fine for the trunk. Thanks for
> > > your patience & explanations.
> > 
> >  Thank you for your review, I have committed this change to trunk now.
> > 
> >  Richard, OK for GCC 11 (correcting a VAX target regression caused by
> > commit 204213fdf23d)?
> 
> OK.

 Backported now, thank you!  Closing issue.

  Maciej