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

[Jeff Law <jeffreyalaw@gmail.com>]

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