Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
This should be covered by PR110308 (proposed fix attached there) and PR110313. Our bootstrap runs are still in progress to confirm. On Thu, 22 Jun 2023 at 09:40, Richard Biener wrote: > > On Thu, Jun 22, 2023 at 1:42 AM Thiago Jung Bauermann > wrote: > > > > > > Hello, > > > > Jeff Law writes: > > > > > On 6/19/23 22:52, Tamar Christina wrote: > > > > > >>> 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. > > > Rejecting for RTX_FRAME_RELATED_P would seem reasonable and probably > > > better in general to > > > me. The cases where we're looking to clean things up aren't really in the > > > prologue/epilogue, but instead in the main body after register > > > elimination has turned fp > > > into sp + offset, thus making all kinds of things no longer valid. > > > > The problems I reported were fixed by commits: > > > > 580b74a79146 "aarch64: Robustify stack tie handling" > > 079f31c55318 "aarch64: Fix gcc.target/aarch64/sve/pcs failures" > > > > Thanks! > > > > But unfortunately I'm still seeing bootstrap failures (ICE segmentation > > fault) in today's trunk with build config bootstrap-lto in both > > armv8l-linux-gnueabihf and aarch64-linux-gnu. > > If there's not yet a bugreport for this please make sure to open one so > this issue doesn't get lost. > > > If I revert commit 6a2e8dcbbd4b "cprop_hardreg: Enable propagation of > > the stack pointer if possible" from trunk then both bootstraps succeed. > > > > Here's the command I'm using to build on armv8l: > > > > ~/src/configure \ > > SHELL=/bin/bash \ > > --with-gnu-as \ > > --with-gnu-ld \ > > --disable-libmudflap \ > > --enable-lto \ > > --enable-shared \ > > --without-included-gettext \ > > --enable-nls \ > > --with-system-zlib \ > > --disable-sjlj-exceptions \ > > --enable-gnu-unique-object \ > > --enable-linker-build-id \ > > --disable-libstdcxx-pch \ > > --enable-c99 \ > > --enable-clocale=gnu \ > > --enable-libstdcxx-debug \ > > --enable-long-long \ > > --with-cloog=no \ > > --with-ppl=no \ > > --with-isl=no \ > > --disable-multilib \ > > --with-float=hard \ > > --with-fpu=neon-fp-armv8 \ > > --with-mode=thumb \ > > --with-arch=armv8-a \ > > --enable-threads=posix \ > > --enable-multiarch \ > > --enable-libstdcxx-time=yes \ > > --enable-gnu-indirect-function \ > > --disable-werror \ > > --enable-checking=yes \ > > --enable-bootstrap \ > > --with-build-config=bootstrap-lto \ > > --enable-languages=c,c++,fortran,lto \ > > && make \ > > profiledbootstrap \ > > SHELL=/bin/bash \ > > -w \ > > -j 40 \ > > CFLAGS_FOR_BUILD="-pipe -g -O2" \ > > CXXFLAGS_FOR_BUILD="-pipe -g -O2" \ > > LDFLAGS_FOR_BUILD="-static-libgcc" \ > > MAKEINFOFLAGS=--force \ > > BUILD_INFO="" \ > > MAKEINFO=echo > > > > And here's the slightly different one for aarch64-linux: > > > > ~/src/configure \ > > SHELL=/bin/bash \ > > --with-gnu-as \ > > --with-gnu-ld \ > > --disable-libmudflap \ > > --enable-lto \ > > --enable-shared \ > > --without-included-gettext \ > > --enable-nls \ > > --with-system-zlib \ > > --disable-sjlj-exceptions \ > > --enable-gnu-unique-object \ > > --enable-linker-build-id \ > > --disable-libstdcxx-pch \ > > --enable-c99 \ > > --enable-clocale=gnu \ > > --enable-libstdcxx-debug \ > > --enable-long-long \ > > --with-cloog=no \ > > --with-ppl=no \ > > --with-isl=no \ > > --disable-multilib \ > > --enable-fix-cortex-a53-835769 \ > > --enable-fix-cortex-a53-843419 \ > > --with-arch=armv8-a \ > > --enable-threads=posix \ > > --enable-multiarch \ > > --enable-libstdcxx-time=yes \ > > --enable-gnu-indirect-function \ > > --disable-werror \ > > --enable-checking=yes \ > > --enable-bootstrap \ > > --with-build-config=bootstrap-lto \ > >
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On Thu, Jun 22, 2023 at 1:42 AM Thiago Jung Bauermann wrote: > > > Hello, > > Jeff Law writes: > > > On 6/19/23 22:52, Tamar Christina wrote: > > > >>> 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. > > Rejecting for RTX_FRAME_RELATED_P would seem reasonable and probably better > > in general to > > me. The cases where we're looking to clean things up aren't really in the > > prologue/epilogue, but instead in the main body after register elimination > > has turned fp > > into sp + offset, thus making all kinds of things no longer valid. > > The problems I reported were fixed by commits: > > 580b74a79146 "aarch64: Robustify stack tie handling" > 079f31c55318 "aarch64: Fix gcc.target/aarch64/sve/pcs failures" > > Thanks! > > But unfortunately I'm still seeing bootstrap failures (ICE segmentation > fault) in today's trunk with build config bootstrap-lto in both > armv8l-linux-gnueabihf and aarch64-linux-gnu. If there's not yet a bugreport for this please make sure to open one so this issue doesn't get lost. > If I revert commit 6a2e8dcbbd4b "cprop_hardreg: Enable propagation of > the stack pointer if possible" from trunk then both bootstraps succeed. > > Here's the command I'm using to build on armv8l: > > ~/src/configure \ > SHELL=/bin/bash \ > --with-gnu-as \ > --with-gnu-ld \ > --disable-libmudflap \ > --enable-lto \ > --enable-shared \ > --without-included-gettext \ > --enable-nls \ > --with-system-zlib \ > --disable-sjlj-exceptions \ > --enable-gnu-unique-object \ > --enable-linker-build-id \ > --disable-libstdcxx-pch \ > --enable-c99 \ > --enable-clocale=gnu \ > --enable-libstdcxx-debug \ > --enable-long-long \ > --with-cloog=no \ > --with-ppl=no \ > --with-isl=no \ > --disable-multilib \ > --with-float=hard \ > --with-fpu=neon-fp-armv8 \ > --with-mode=thumb \ > --with-arch=armv8-a \ > --enable-threads=posix \ > --enable-multiarch \ > --enable-libstdcxx-time=yes \ > --enable-gnu-indirect-function \ > --disable-werror \ > --enable-checking=yes \ > --enable-bootstrap \ > --with-build-config=bootstrap-lto \ > --enable-languages=c,c++,fortran,lto \ > && make \ > profiledbootstrap \ > SHELL=/bin/bash \ > -w \ > -j 40 \ > CFLAGS_FOR_BUILD="-pipe -g -O2" \ > CXXFLAGS_FOR_BUILD="-pipe -g -O2" \ > LDFLAGS_FOR_BUILD="-static-libgcc" \ > MAKEINFOFLAGS=--force \ > BUILD_INFO="" \ > MAKEINFO=echo > > And here's the slightly different one for aarch64-linux: > > ~/src/configure \ > SHELL=/bin/bash \ > --with-gnu-as \ > --with-gnu-ld \ > --disable-libmudflap \ > --enable-lto \ > --enable-shared \ > --without-included-gettext \ > --enable-nls \ > --with-system-zlib \ > --disable-sjlj-exceptions \ > --enable-gnu-unique-object \ > --enable-linker-build-id \ > --disable-libstdcxx-pch \ > --enable-c99 \ > --enable-clocale=gnu \ > --enable-libstdcxx-debug \ > --enable-long-long \ > --with-cloog=no \ > --with-ppl=no \ > --with-isl=no \ > --disable-multilib \ > --enable-fix-cortex-a53-835769 \ > --enable-fix-cortex-a53-843419 \ > --with-arch=armv8-a \ > --enable-threads=posix \ > --enable-multiarch \ > --enable-libstdcxx-time=yes \ > --enable-gnu-indirect-function \ > --disable-werror \ > --enable-checking=yes \ > --enable-bootstrap \ > --with-build-config=bootstrap-lto \ > --enable-languages=c,c++,fortran,lto \ > && make \ > profiledbootstrap \ > SHELL=/bin/bash \ > -w \ > -j 40 \ > LDFLAGS_FOR_TARGET="-Wl,-fix-cortex-a53-843419" \ > CFLAGS_FOR_BUILD="-pipe -g -O2" \ > CXXFLAGS_FOR_BUILD="-pipe -g -O2" \ > LDFLAGS_FOR_BUILD="-static-libgcc" \ > MAKEINFOFLAGS=--force \ > BUILD_INFO="" \ > MAKEINFO=echo > > -- > Thiago
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
Hello, Jeff Law writes: > On 6/19/23 22:52, Tamar Christina wrote: > >>> 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. > Rejecting for RTX_FRAME_RELATED_P would seem reasonable and probably better > in general to > me. The cases where we're looking to clean things up aren't really in the > prologue/epilogue, but instead in the main body after register elimination > has turned fp > into sp + offset, thus making all kinds of things no longer valid. The problems I reported were fixed by commits: 580b74a79146 "aarch64: Robustify stack tie handling" 079f31c55318 "aarch64: Fix gcc.target/aarch64/sve/pcs failures" Thanks! But unfortunately I'm still seeing bootstrap failures (ICE segmentation fault) in today's trunk with build config bootstrap-lto in both armv8l-linux-gnueabihf and aarch64-linux-gnu. If I revert commit 6a2e8dcbbd4b "cprop_hardreg: Enable propagation of the stack pointer if possible" from trunk then both bootstraps succeed. Here's the command I'm using to build on armv8l: ~/src/configure \ SHELL=/bin/bash \ --with-gnu-as \ --with-gnu-ld \ --disable-libmudflap \ --enable-lto \ --enable-shared \ --without-included-gettext \ --enable-nls \ --with-system-zlib \ --disable-sjlj-exceptions \ --enable-gnu-unique-object \ --enable-linker-build-id \ --disable-libstdcxx-pch \ --enable-c99 \ --enable-clocale=gnu \ --enable-libstdcxx-debug \ --enable-long-long \ --with-cloog=no \ --with-ppl=no \ --with-isl=no \ --disable-multilib \ --with-float=hard \ --with-fpu=neon-fp-armv8 \ --with-mode=thumb \ --with-arch=armv8-a \ --enable-threads=posix \ --enable-multiarch \ --enable-libstdcxx-time=yes \ --enable-gnu-indirect-function \ --disable-werror \ --enable-checking=yes \ --enable-bootstrap \ --with-build-config=bootstrap-lto \ --enable-languages=c,c++,fortran,lto \ && make \ profiledbootstrap \ SHELL=/bin/bash \ -w \ -j 40 \ CFLAGS_FOR_BUILD="-pipe -g -O2" \ CXXFLAGS_FOR_BUILD="-pipe -g -O2" \ LDFLAGS_FOR_BUILD="-static-libgcc" \ MAKEINFOFLAGS=--force \ BUILD_INFO="" \ MAKEINFO=echo And here's the slightly different one for aarch64-linux: ~/src/configure \ SHELL=/bin/bash \ --with-gnu-as \ --with-gnu-ld \ --disable-libmudflap \ --enable-lto \ --enable-shared \ --without-included-gettext \ --enable-nls \ --with-system-zlib \ --disable-sjlj-exceptions \ --enable-gnu-unique-object \ --enable-linker-build-id \ --disable-libstdcxx-pch \ --enable-c99 \ --enable-clocale=gnu \ --enable-libstdcxx-debug \ --enable-long-long \ --with-cloog=no \ --with-ppl=no \ --with-isl=no \ --disable-multilib \ --enable-fix-cortex-a53-835769 \ --enable-fix-cortex-a53-843419 \ --with-arch=armv8-a \ --enable-threads=posix \ --enable-multiarch \ --enable-libstdcxx-time=yes \ --enable-gnu-indirect-function \ --disable-werror \ --enable-checking=yes \ --enable-bootstrap \ --with-build-config=bootstrap-lto \ --enable-languages=c,c++,fortran,lto \ && make \ profiledbootstrap \ SHELL=/bin/bash \ -w \ -j 40 \ LDFLAGS_FOR_TARGET="-Wl,-fix-cortex-a53-843419" \ CFLAGS_FOR_BUILD="-pipe -g -O2" \ CXXFLAGS_FOR_BUILD="-pipe -g -O2" \ LDFLAGS_FOR_BUILD="-static-libgcc" \ MAKEINFOFLAGS=--force \ BUILD_INFO="" \ MAKEINFO=echo -- Thiago
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On 6/19/23 22:52, Tamar Christina wrote: 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. Rejecting for RTX_FRAME_RELATED_P would seem reasonable and probably better in general to me. The cases where we're looking to clean things up aren't really in the prologue/epilogue, but instead in the main body after register elimination has turned fp into sp + offset, thus making all kinds of things no longer valid. jeff
RE: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
> -Original Message-
> From: Jeff Law
> Sent: Tuesday, June 20, 2023 3:17 AM
> To: Andrew Pinski ; Thiago Jung Bauermann
>
> Cc: Manolis Tsamis ; Philipp Tomsich
> ; Richard Biener ;
> Palmer Dabbelt ; Kito Cheng ;
> gcc-patches@gcc.gnu.org; Tamar Christina
> 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
> wrote:
> >>
> >> On Mon, Jun 19, 2023 at 9:58 AM Thiago Jung Bauermann via Gcc-patches
> >> wrote:
> >>>
> >>>
> >>> Hello Manolis,
> >>>
> >>> Philipp Tomsich writes:
> >>>
> >>>> On Thu, 8 Jun 2023 at 00:18, Jeff Law 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:
> > ```
> >
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 wrote:
On Mon, Jun 19, 2023 at 9:58 AM Thiago Jung Bauermann via Gcc-patches
wrote:
Hello Manolis,
Philipp Tomsich writes:
On Thu, 8 Jun 2023 at 00:18, Jeff Law 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
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On Mon, Jun 19, 2023 at 4:40 PM Andrew Pinski wrote:
>
> On Mon, Jun 19, 2023 at 9:58 AM Thiago Jung Bauermann via Gcc-patches
> wrote:
> >
> >
> > Hello Manolis,
> >
> > Philipp Tomsich writes:
> >
> > > On Thu, 8 Jun 2023 at 00:18, Jeff Law 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.
Thanks,
Andrew
>
> Thanks,
> Andrew Pinski
>
>
> >
> > Running gcc:gcc.target/aarch64/sve/pcs/aarch64-sve-pcs.exp ...
> > FAIL: gcc.target/aarch64/sve/pcs/args_1.c -march=armv8.2-a+sve
> > -fno-stack-protector check-function-bodies caller_pred
> > FAIL: gcc.target/aarch64/sve/pcs/args_2.c -march=armv8.2-a+sve
> > -fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
> > #8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
> > FAIL: gcc.target/aarch64/sve/pcs/args_3.c -march=armv8.2-a+sve
> > -fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
> > #8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
> > FAIL: gcc.target/aarch64/sve/pcs/args_4.c -march=armv8.2-a+sve
> > -fno-stack-protector scan-assembler \\tfmov\\t(z[0-9]+\\.h),
> > #8\\.0.*\\tst1h\\t\\1, p[0-7], \\[x4\\]\\n
> > FAIL:
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On Mon, Jun 19, 2023 at 9:58 AM Thiago Jung Bauermann via Gcc-patches
wrote:
>
>
> Hello Manolis,
>
> Philipp Tomsich writes:
>
> > On Thu, 8 Jun 2023 at 00:18, Jeff Law 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.
Thanks,
Andrew Pinski
>
> Running gcc:gcc.target/aarch64/sve/pcs/aarch64-sve-pcs.exp ...
> FAIL: gcc.target/aarch64/sve/pcs/args_1.c -march=armv8.2-a+sve
> -fno-stack-protector check-function-bodies caller_pred
> FAIL: gcc.target/aarch64/sve/pcs/args_2.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
> #8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_3.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
> #8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_4.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tfmov\\t(z[0-9]+\\.h),
> #8\\.0.*\\tst1h\\t\\1, p[0-7], \\[x4\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_bf16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f32.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
> z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f64.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
> z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s32.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
> z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s64.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
> z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s8.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2b\\t{(z[0-9]+\\.b) -
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On Mon, Jun 19, 2023 at 7:57 PM Thiago Jung Bauermann
wrote:
>
>
> Hello Manolis,
>
> Philipp Tomsich writes:
>
> > On Thu, 8 Jun 2023 at 00:18, Jeff Law 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
>
> Running gcc:gcc.target/aarch64/sve/pcs/aarch64-sve-pcs.exp ...
> FAIL: gcc.target/aarch64/sve/pcs/args_1.c -march=armv8.2-a+sve
> -fno-stack-protector check-function-bodies caller_pred
> FAIL: gcc.target/aarch64/sve/pcs/args_2.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
> #8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_3.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
> #8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_4.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tfmov\\t(z[0-9]+\\.h),
> #8\\.0.*\\tst1h\\t\\1, p[0-7], \\[x4\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_bf16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f32.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
> z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f64.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
> z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s32.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
> z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s64.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
> z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s8.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2b\\t{(z[0-9]+\\.b) -
> z[0-9]+\\.b}.*\\tst1b\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
> z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u32.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
> z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u64.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
> z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u8.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2b\\t{(z[0-9]+\\.b) -
> z[0-9]+\\.b}.*\\tst1b\\t\\1, p[0-7], \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_le_bf16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+)\\.h -
> z[0-9]+\\.h}.*\\tstr\\t\\1, \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_le_f16.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+)\\.h -
> z[0-9]+\\.h}.*\\tstr\\t\\1, \\[x1\\]\\n
> FAIL: gcc.target/aarch64/sve/pcs/args_5_le_f32.c -march=armv8.2-a+sve
> -fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+)\\.s -
> z[0-9]+\\.s}.*\\tstr\\t\\1, \\[x1\\]\\n
> FAIL:
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
Hello Manolis,
Philipp Tomsich writes:
> On Thu, 8 Jun 2023 at 00:18, Jeff Law 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
Running gcc:gcc.target/aarch64/sve/pcs/aarch64-sve-pcs.exp ...
FAIL: gcc.target/aarch64/sve/pcs/args_1.c -march=armv8.2-a+sve
-fno-stack-protector check-function-bodies caller_pred
FAIL: gcc.target/aarch64/sve/pcs/args_2.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
#8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_3.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tmov\\t(z[0-9]+\\.b),
#8\\n.*\\tst1b\\t\\1, p[0-7], \\[x4\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_4.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tfmov\\t(z[0-9]+\\.h),
#8\\.0.*\\tst1h\\t\\1, p[0-7], \\[x4\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_bf16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f32.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_f64.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s32.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s64.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_s8.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2b\\t{(z[0-9]+\\.b) -
z[0-9]+\\.b}.*\\tst1b\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+\\.h) -
z[0-9]+\\.h}.*\\tst1h\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u32.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+\\.s) -
z[0-9]+\\.s}.*\\tst1w\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u64.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+\\.d) -
z[0-9]+\\.d}.*\\tst1d\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_be_u8.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2b\\t{(z[0-9]+\\.b) -
z[0-9]+\\.b}.*\\tst1b\\t\\1, p[0-7], \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_le_bf16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+)\\.h -
z[0-9]+\\.h}.*\\tstr\\t\\1, \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_le_f16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2h\\t{(z[0-9]+)\\.h -
z[0-9]+\\.h}.*\\tstr\\t\\1, \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_le_f32.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2w\\t{(z[0-9]+)\\.s -
z[0-9]+\\.s}.*\\tstr\\t\\1, \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_le_f64.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler \\tld2d\\t{(z[0-9]+)\\.d -
z[0-9]+\\.d}.*\\tstr\\t\\1, \\[x1\\]\\n
FAIL: gcc.target/aarch64/sve/pcs/args_5_le_s16.c -march=armv8.2-a+sve
-fno-stack-protector scan-assembler
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
Rebased, retested, and applied to trunk. Thanks! --Philipp. On Thu, 8 Jun 2023 at 00:18, Jeff Law 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. > > jeff
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
Hi Jeff, Yes that one has changed; I changed the implementation based on your feedback. Thanks, Manolis On Thu, Jun 8, 2023 at 1:18 AM Jeff Law 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. > > jeff
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
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. jeff
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On 5/31/23 06:15, Manolis Tsamis wrote: On Thu, May 25, 2023 at 4:38 PM Jeff Law 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. I can't see how this can be correct given the stack pointer equality tests elsewhere in the compiler, particularly the various targets. The problem is if you change the mode then you end up with multiple REG expressions that reference the stack pointer. See rev: d1446456c3fcaa7be628726c9de4a877729490ca and the thread around the change which introduced this code. Hi Jeff, Isn't this fine for this case since: 1) stack_pointer_rtx is used which won't cause issues with pointer equalities (If I understand correctly). 2) Propagation is guarded with `if (orig_mode == new_mode)` so only when there is no mode change. I must have missed #2 -- is that something that changed since the first iteration for Ventana many months ago? Anyway, hoping to make meaningful progress on these two patches over the next couple days. jeff
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
On Thu, May 25, 2023 at 4:38 PM Jeff Law 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. > I can't see how this can be correct given the stack pointer equality > tests elsewhere in the compiler, particularly the various targets. > > The problem is if you change the mode then you end up with multiple REG > expressions that reference the stack pointer. > > See rev: d1446456c3fcaa7be628726c9de4a877729490ca and the thread around > the change which introduced this code. > Hi Jeff, Isn't this fine for this case since: 1) stack_pointer_rtx is used which won't cause issues with pointer equalities (If I understand correctly). 2) Propagation is guarded with `if (orig_mode == new_mode)` so only when there is no mode change. Thanks, Manolis > > Jeff
Re: [PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
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. I can't see how this can be correct given the stack pointer equality tests elsewhere in the compiler, particularly the various targets. The problem is if you change the mode then you end up with multiple REG expressions that reference the stack pointer. See rev: d1446456c3fcaa7be628726c9de4a877729490ca and the thread around the change which introduced this code. Jeff
[PATCH 2/2] cprop_hardreg: Enable propagation of the stack pointer if possible.
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.
Signed-off-by: Manolis Tsamis
---
gcc/regcprop.cc | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/gcc/regcprop.cc b/gcc/regcprop.cc
index f426f4fedcd..6cbfadb181f 100644
--- a/gcc/regcprop.cc
+++ b/gcc/regcprop.cc
@@ -422,7 +422,12 @@ maybe_mode_change (machine_mode orig_mode, machine_mode
copy_mode,
It's unclear if we need to do the same for other special registers. */
if (regno == STACK_POINTER_REGNUM)
-return NULL_RTX;
+{
+ if (orig_mode == new_mode)
+ return stack_pointer_rtx;
+ else
+ return NULL_RTX;
+}
if (orig_mode == new_mode)
return gen_raw_REG (new_mode, regno);
--
2.34.1
