On 6/19/23 17:48, Andrew Pinski wrote:
On Mon, Jun 19, 2023 at 4:40 PM Andrew Pinski <pins...@gmail.com> wrote:
On Mon, Jun 19, 2023 at 9:58 AM Thiago Jung Bauermann via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
Hello Manolis,
Philipp Tomsich <philipp.toms...@vrull.eu> writes:
On Thu, 8 Jun 2023 at 00:18, Jeff Law <jeffreya...@gmail.com> wrote:
On 5/25/23 06:35, Manolis Tsamis wrote:
Propagation of the stack pointer in cprop_hardreg is currenty forbidden
in all cases, due to maybe_mode_change returning NULL. Relax this
restriction and allow propagation when no mode change is requested.
gcc/ChangeLog:
* regcprop.cc (maybe_mode_change): Enable stack pointer propagation.
Thanks for the clarification. This is OK for the trunk. It looks
generic enough to have value going forward now rather than waiting.
Rebased, retested, and applied to trunk. Thanks!
Our CI found a couple of tests that started failing on aarch64-linux
after this commit. I was able to confirm manually that they don't happen
in the commit immediately before this one, and also that these failures
are still present in today's trunk.
I have testsuite logs for last good commit, first bad commit and current
trunk here:
https://people.linaro.org/~thiago.bauermann/gcc-regression-6a2e8dcbbd4b/
Could you please check?
These are the new failures:
Running gcc:gcc.target/aarch64/aarch64.exp ...
FAIL: gcc.target/aarch64/stack-check-cfa-3.c scan-assembler-times mov\\tx11, sp
1
So for the above before this change we had:
```
(insn:TI 597 596 598 2 (set (reg:DI 11 x11)
(reg/f:DI 31 sp)) "stack-check-prologue-16.c":16:1 65 {*movdi_aarch64}
(nil))
(insn 598 597 599 2 (set (mem:BLK (scratch) [0 A8])
(unspec:BLK [
(reg:DI 11 x11)
(reg/f:DI 31 sp)
] UNSPEC_PRLG_STK)) "stack-check-prologue-16.c":16:1 1169
{stack_tie}
(expr_list:REG_DEAD (reg:DI 11 x11)
(nil)))
```
After we get:
```
(insn 598 596 599 2 (set (mem:BLK (scratch) [0 A8])
(unspec:BLK [
(reg:DI 31 sp [11]) repeated x2
] UNSPEC_PRLG_STK)) "stack-check-prologue-16.c":16:1 1169
{stack_tie}
(nil))
```
Which seems to be ok, except we still have:
.cfi_def_cfa_register 11
That is because on:
(insn/f 596 595 598 2 (set (reg:DI 12 x12)
(plus:DI (reg:DI 12 x12)
(const_int 272 [0x110]))) "stack-check-prologue-16.c":16:1
153 {*adddi3_aarch64}
(expr_list:REG_CFA_DEF_CFA (reg:DI 11 x11)
(nil)))
We record x11 but never update it though that came before the mov for
x11 ... So it seems like cprop_hardreg had no idea it needed to update
it.
I suspect the other testcases are just propagation of sp into the
stores and such and just needed update. But the above testcase seems
getting broken cfi though I don't know how to fix it.
The code from aarch64.cc:
```
/* This is done to provide unwinding information for the stack
adjustments we're about to do, however to prevent the optimizers
from removing the R11 move and leaving the CFA note (which would
be
very wrong) we tie the old and new stack pointer together.
The tie will expand to nothing but the optimizers will not touch
the instruction. */
rtx stack_ptr_copy = gen_rtx_REG (Pmode, STACK_CLASH_SVE_CFA_REGNUM);
emit_move_insn (stack_ptr_copy, stack_pointer_rtx);
emit_insn (gen_stack_tie (stack_ptr_copy, stack_pointer_rtx));
/* We want the CFA independent of the stack pointer for the
duration of the loop. */
add_reg_note (insn, REG_CFA_DEF_CFA, stack_ptr_copy);
RTX_FRAME_RELATED_P (insn) = 1;
```
Well except now with this change, the optimizers touch this
instruction. Maybe the move instruction should not be a move but an
unspec so optimizers don't know what the move was.
Adding Tamar to the CC who added this code to aarch64 originally for
comments on the above understanding here.
It's a bit hackish, but could we reject the stack pointer for operand1
in the stack-tie? And if we do so, does it help?
jeff
