Re: [PATCH] PR middle-end/103059: reload: Also accept ASHIFT with indexed addressing

["Maciej W. Rozycki" <macro@embecosm.com>]

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