RE: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Jeff Law <jeffreyalaw@gmail.com>
> Sent: Tuesday, June 20, 2023 3:17 AM
> To: Andrew Pinski <pinskia@gmail.com>; Thiago Jung Bauermann
> <thiago.bauermann@linaro.org>
> Cc: Manolis Tsamis <manolis.tsamis@vrull.eu>; Philipp Tomsich
> <philipp.tomsich@vrull.eu>; Richard Biener <richard.guenther@gmail.com>;
> Palmer Dabbelt <palmer@rivosinc.com>; Kito Cheng <kito.cheng@gmail.com>;
> gcc-patches@gcc.gnu.org; Tamar Christina <Tamar.Christina@arm.com>
> Subject: Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack
> pointer if possible.
> 
> 
> 
> On 6/19/23 17:48, Andrew Pinski wrote:
> > On Mon, Jun 19, 2023 at 4:40 PM Andrew Pinski <pinskia@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.tomsich@vrull.eu> writes:
> >>>
> >>>> On Thu, 8 Jun 2023 at 00:18, Jeff Law <jeffreyalaw@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-6a2e8dcbb
> >>> d4b/
> >>>
> >>> 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.

Yeah, we noticed the failures internally but left them broken since we have an
upcoming AArch64 patch which requires them to be updated anyway and are
rolling up the updates into that patch. 

> >
> > 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?

Yeah this one I had to defer until later this week to look at closer because what I'm
wondering about is whether the optimization should apply to frame related
RTX as well.

Looking at the description of RTX_FRAME_RELATED_P that this optimization may
end up de-optimizing RISC targets by creating an offset that is larger than offset
which can be used from a SP making reload having to spill.  i.e. sometimes the
move was explicitly done. So perhaps it should not apply it to
RTX_FRAME_RELATED_P in find_oldest_value_reg and copyprop_hardreg_forward_1?

Other parts of this pass already seems to bail out in similar situations.   So I needed to
write some testcases to check what would happen in these cases hence the deferral.
to later in the week.

Kind Regards,
Tamar

> 
> jeff