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

[Jeff Law <jeffreyalaw@gmail.com>]

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