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
