diff mbox series

[alpha] adjust MEM alignment for block move [PR115459] (was: Re: [PATCH v2] [PR100106] Reject unaligned subregs when strict alignment is required)

Message ID ory179fg4z.fsf_-_@lxoliva.fsfla.org
State New
Headers show
Series [alpha] adjust MEM alignment for block move [PR115459] (was: Re: [PATCH v2] [PR100106] Reject unaligned subregs when strict alignment is required) | expand

Commit Message

Alexandre Oliva June 13, 2024, 7:36 a.m. UTC 
Hello, Maciej,

On Jun 12, 2024, "Maciej W. Rozycki" <macro@orcam.me.uk> wrote:

>  This has regressed building the `alpha-linux-gnu' target, in libada, as 
> from commit d6b756447cd5 including GCC 14 and up to current GCC 15 trunk:

> | Error detected around g-debpoo.adb:1896:8                                |

> I have filed PR #115459.

Thanks!

This was tricky to duplicate without access to an alpha-linux-gnu
machine.  I ended up building an uberbaum tree with --disable-shared
--disable-threads --enable-languages=ada up to all-target-libgcc, then I
replaced gcc/collect2 with a wrapper script that dropped crt[1in].o and
-lc, so that link tests in libada/configure would succeed without glibc
for the target.  libada still wouldn't build, because of the missing
glibc headers, but I could compile g-depboo.adb with -I pointing at a
x86_64-linux-gnu's gcc/ada/rts build tree, and with that, at -O2, I
could trigger the problem and investigate it.  And with the following
patch, the problem seems to be gone.

Maciej, would you be so kind as to give it a spin with a native
regstrap?  TIA,

Richard, is this ok to install if regstrapping succeeds?


Before issuing loads or stores for a block move, adjust the MEM
alignments if analysis of the addresses enabled the inference of
stricter alignment.  This ensures that the MEMs are sufficiently
aligned for the corresponding insns, which avoids trouble in case of
e.g. substitutions into SUBREGs.


for  gcc/ChangeLog

	PR target/115459
	* config/alpha/alpha.cc (alpha_expand_block_move): Adjust
	MEMs to match inferred alignment.
---
 gcc/config/alpha/alpha.cc |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Maciej W. Rozycki June 13, 2024, 11:49 a.m. UTC | #1 
Hi Alexandre,

> >  This has regressed building the `alpha-linux-gnu' target, in libada, as 
> > from commit d6b756447cd5 including GCC 14 and up to current GCC 15 trunk:
> 
> > | Error detected around g-debpoo.adb:1896:8                                |
> 
> > I have filed PR #115459.
> 
> Thanks!
> 
> This was tricky to duplicate without access to an alpha-linux-gnu
> machine.  I ended up building an uberbaum tree with --disable-shared
> --disable-threads --enable-languages=ada up to all-target-libgcc, then I
> replaced gcc/collect2 with a wrapper script that dropped crt[1in].o and
> -lc, so that link tests in libada/configure would succeed without glibc
> for the target.  libada still wouldn't build, because of the missing
> glibc headers, but I could compile g-depboo.adb with -I pointing at a
> x86_64-linux-gnu's gcc/ada/rts build tree, and with that, at -O2, I
> could trigger the problem and investigate it.  And with the following
> patch, the problem seems to be gone.

 If you like, I'll share with you a pair of scripts I use to cross-compile 
Linux configurations.  No target system is required to build things and 
all is done based on git checkouts from the relevant upstream repos, which 
then bootstrap themselves using whatever compiler is locally available to 
bootstrap native GCC first and then going through the ususal steps to get 
a target cross-compiler.

> Maciej, would you be so kind as to give it a spin with a native
> regstrap?  TIA,

 I will certainly run regression-testing once the job I started yesterday 
has finished with my Alpha system, which should be fairly soon as it's 
already well into libstdc++ testing.

 I cannot make a native bootstrap however as I have only just set up my 
Alpha to run at all and it has a very rudimentary and outdated userland, 
suitable for remote regression testing only.  It's NFS-rooted too and due 
to a failure in my lab last month I may not be able to recover from before 
August it runs over backup 10Mbps Ethernet rather than intended 100Mbps 
FDDI, so I can imagine performance would be abysmal even if I brought the 
userland up to date.

 However Adrian (cc-ed) has recently told me he could be running all kinds 
of stuff with his Alpha.  Adrian, would you be able to verify Alexandre's 
proposed fix in a native regstrap?

 Thanks a lot for such a quick fix, and then making it in the first place!

  Maciej
Joseph Myers June 13, 2024, 3:21 p.m. UTC | #2 
On Thu, 13 Jun 2024, Maciej W. Rozycki wrote:

> > This was tricky to duplicate without access to an alpha-linux-gnu
> > machine.  I ended up building an uberbaum tree with --disable-shared
> > --disable-threads --enable-languages=ada up to all-target-libgcc, then I
> > replaced gcc/collect2 with a wrapper script that dropped crt[1in].o and
> > -lc, so that link tests in libada/configure would succeed without glibc
> > for the target.  libada still wouldn't build, because of the missing
> > glibc headers, but I could compile g-depboo.adb with -I pointing at a
> > x86_64-linux-gnu's gcc/ada/rts build tree, and with that, at -O2, I
> > could trigger the problem and investigate it.  And with the following
> > patch, the problem seems to be gone.
> 
>  If you like, I'll share with you a pair of scripts I use to cross-compile 
> Linux configurations.  No target system is required to build things and 
> all is done based on git checkouts from the relevant upstream repos, which 
> then bootstrap themselves using whatever compiler is locally available to 
> bootstrap native GCC first and then going through the ususal steps to get 
> a target cross-compiler.

Also, you can use build-many-glibcs.py --full-gcc to build a cross 
compiler with all languages (for any architecture and ABI supported by 
glibc), but you need to build the same-version native compiler yourself 
first (and put it in the PATH) in order to build Ada; that bit isn't 
automated by build-many-glibcs.py.
Maciej W. Rozycki June 18, 2024, 9:59 a.m. UTC | #3 
On Thu, 13 Jun 2024, Maciej W. Rozycki wrote:

> > Maciej, would you be so kind as to give it a spin with a native
> > regstrap?  TIA,
> 
>  I will certainly run regression-testing once the job I started yesterday 
> has finished with my Alpha system, which should be fairly soon as it's 
> already well into libstdc++ testing.

 This has now completed successfully, with no regressions observed across 
all the GCC and library testsuites other than the gnat one.

 The gnat one obviously couldn't be run without the fix in place, not even 
in a build without libada, because it depends on gnattools, which in turn 
need libada.  Results with the change applied appear reasonable however, 
though they are not completely clean.  I trigerred issues while running 
this part of the testsuite and I have now posted a proposed fix, at: 
<https://inbox.sourceware.org/gcc-patches/alpine.DEB.2.21.2406161338480.9248@angie.orcam.me.uk/T/>.

 Also one of the libstdc++ test cases caused to lock the target machine up 
regardless of the fix due to memory exhaustion which took me some time to 
investigate and sort out (now dealt with `ulimit -d'), and a testsuite run 
takes 24 hours almost exactly, hence the total amount of time it took me 
to complete this verification.

>  I cannot make a native bootstrap however as I have only just set up my 
> Alpha to run at all and it has a very rudimentary and outdated userland, 
> suitable for remote regression testing only.  It's NFS-rooted too and due 
> to a failure in my lab last month I may not be able to recover from before 
> August it runs over backup 10Mbps Ethernet rather than intended 100Mbps 
> FDDI, so I can imagine performance would be abysmal even if I brought the 
> userland up to date.
> 
>  However Adrian (cc-ed) has recently told me he could be running all kinds 
> of stuff with his Alpha.  Adrian, would you be able to verify Alexandre's 
> proposed fix in a native regstrap?

 We've exchanged with Adrian a couple of messages off-list and I'm not 
sure after all whether he'll be able to run such a regstrap.  Given that 
this fix has addressed a problem affecting building a part of the compiler 
itself and the overall status of the Alpha port I do hope it can go ahead 
based on cross-compilation verification only.  Several of our targets are 
routinely verified in such a manner only.

 FAOD I run my builds with `--enable-werror-always' requested to have 
`-Werror' included in the compilation options just as a native regstrap 
has, so this part of verification has been covered as well.

  Maciej
Maciej W. Rozycki July 1, 2024, 10:33 a.m. UTC | #4 
On Thu, 13 Jun 2024, Alexandre Oliva wrote:

> Before issuing loads or stores for a block move, adjust the MEM
> alignments if analysis of the addresses enabled the inference of
> stricter alignment.  This ensures that the MEMs are sufficiently
> aligned for the corresponding insns, which avoids trouble in case of
> e.g. substitutions into SUBREGs.

 Ping for:

<https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654463.html>,
<https://patchwork.sourceware.org/project/gcc/patch/ory179fg4z.fsf_-_@lxoliva.fsfla.org/>.

  Maciej
Uros Bizjak July 10, 2024, 7:22 a.m. UTC | #5 
On Thu, Jun 13, 2024 at 9:37 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> Hello, Maciej,
>
> On Jun 12, 2024, "Maciej W. Rozycki" <macro@orcam.me.uk> wrote:
>
> >  This has regressed building the `alpha-linux-gnu' target, in libada, as
> > from commit d6b756447cd5 including GCC 14 and up to current GCC 15 trunk:
>
> > | Error detected around g-debpoo.adb:1896:8                                |
>
> > I have filed PR #115459.
>
> Thanks!
>
> This was tricky to duplicate without access to an alpha-linux-gnu
> machine.  I ended up building an uberbaum tree with --disable-shared
> --disable-threads --enable-languages=ada up to all-target-libgcc, then I
> replaced gcc/collect2 with a wrapper script that dropped crt[1in].o and
> -lc, so that link tests in libada/configure would succeed without glibc
> for the target.  libada still wouldn't build, because of the missing
> glibc headers, but I could compile g-depboo.adb with -I pointing at a
> x86_64-linux-gnu's gcc/ada/rts build tree, and with that, at -O2, I
> could trigger the problem and investigate it.  And with the following
> patch, the problem seems to be gone.
>
> Maciej, would you be so kind as to give it a spin with a native
> regstrap?  TIA,
>
> Richard, is this ok to install if regstrapping succeeds?
>
>
> Before issuing loads or stores for a block move, adjust the MEM
> alignments if analysis of the addresses enabled the inference of
> stricter alignment.  This ensures that the MEMs are sufficiently
> aligned for the corresponding insns, which avoids trouble in case of
> e.g. substitutions into SUBREGs.
>
>
> for  gcc/ChangeLog
>
>         PR target/115459
>         * config/alpha/alpha.cc (alpha_expand_block_move): Adjust
>         MEMs to match inferred alignment.

LGTM, based on a successful bootstrap/regtest report down the reply thread.

Thanks,
Uros.

> ---
>  gcc/config/alpha/alpha.cc |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/gcc/config/alpha/alpha.cc b/gcc/config/alpha/alpha.cc
> index 1126cea1f7ba2..e090e74b9d073 100644
> --- a/gcc/config/alpha/alpha.cc
> +++ b/gcc/config/alpha/alpha.cc
> @@ -3820,6 +3820,12 @@ alpha_expand_block_move (rtx operands[])
>            else if (a >= 16 && c % 2 == 0)
>             src_align = 16;
>         }
> +
> +      if (MEM_P (orig_src) && MEM_ALIGN (orig_src) < src_align)
> +       {
> +         orig_src = shallow_copy_rtx (orig_src);
> +         set_mem_align (orig_src, src_align);
> +       }
>      }
>
>    tmp = XEXP (orig_dst, 0);
> @@ -3841,6 +3847,12 @@ alpha_expand_block_move (rtx operands[])
>            else if (a >= 16 && c % 2 == 0)
>             dst_align = 16;
>         }
> +
> +      if (MEM_P (orig_dst) && MEM_ALIGN (orig_dst) < dst_align)
> +       {
> +         orig_dst = shallow_copy_rtx (orig_dst);
> +         set_mem_align (orig_dst, dst_align);
> +       }
>      }
>
>    ofs = 0;
>
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Maciej W. Rozycki July 15, 2024, 12:09 p.m. UTC | #6 
On Wed, 10 Jul 2024, Uros Bizjak wrote:

> > Before issuing loads or stores for a block move, adjust the MEM
> > alignments if analysis of the addresses enabled the inference of
> > stricter alignment.  This ensures that the MEMs are sufficiently
> > aligned for the corresponding insns, which avoids trouble in case of
> > e.g. substitutions into SUBREGs.
> >
> >
> > for  gcc/ChangeLog
> >
> >         PR target/115459
> >         * config/alpha/alpha.cc (alpha_expand_block_move): Adjust
> >         MEMs to match inferred alignment.
> 
> LGTM, based on a successful bootstrap/regtest report down the reply thread.

 Uros, thank you for your review.

 Jakub, is it OK to backport this to fit with the upcoming 14.2 release?

  Maciej
Jakub Jelinek July 15, 2024, 12:13 p.m. UTC | #7 
On Mon, Jul 15, 2024 at 01:09:02PM +0100, Maciej W. Rozycki wrote:
> On Wed, 10 Jul 2024, Uros Bizjak wrote:
> 
> > > Before issuing loads or stores for a block move, adjust the MEM
> > > alignments if analysis of the addresses enabled the inference of
> > > stricter alignment.  This ensures that the MEMs are sufficiently
> > > aligned for the corresponding insns, which avoids trouble in case of
> > > e.g. substitutions into SUBREGs.
> > >
> > >
> > > for  gcc/ChangeLog
> > >
> > >         PR target/115459
> > >         * config/alpha/alpha.cc (alpha_expand_block_move): Adjust
> > >         MEMs to match inferred alignment.
> > 
> > LGTM, based on a successful bootstrap/regtest report down the reply thread.
> 
>  Uros, thank you for your review.
> 
>  Jakub, is it OK to backport this to fit with the upcoming 14.2 release?

14 branch is still normally open for regression bugfixes and documentation
fixes, so patches which were approved and satisfy the backport criteria can
be committed without extra approval from RMs.
14 branch will be only frozen on July 23rd; the only currently frozen branch
is 11.

	Jakub
Maciej W. Rozycki July 15, 2024, 9:11 p.m. UTC | #8 
On Mon, 15 Jul 2024, Jakub Jelinek wrote:

> > > > for  gcc/ChangeLog
> > > >
> > > >         PR target/115459
> > > >         * config/alpha/alpha.cc (alpha_expand_block_move): Adjust
> > > >         MEMs to match inferred alignment.
> > > 
> > > LGTM, based on a successful bootstrap/regtest report down the reply thread.
> > 
> >  Uros, thank you for your review.
> > 
> >  Jakub, is it OK to backport this to fit with the upcoming 14.2 release?
> 
> 14 branch is still normally open for regression bugfixes and documentation
> fixes, so patches which were approved and satisfy the backport criteria can
> be committed without extra approval from RMs.
> 14 branch will be only frozen on July 23rd; the only currently frozen branch
> is 11.

 Thank you.  It's Alexandre's change though, so I'll leave it up to him to 
decide when to commit.

  Maciej
Alexandre Oliva July 16, 2024, 11:29 a.m. UTC | #9 
On Jul 15, 2024, "Maciej W. Rozycki" <macro@orcam.me.uk> wrote:

>  Thank you.  It's Alexandre's change though, so I'll leave it up to him to 
> decide when to commit.

I'm going ahead and blindly putting it in, with the caveat that testing
on alpha remains a challenge for me.
diff mbox series

Patch

diff --git a/gcc/config/alpha/alpha.cc b/gcc/config/alpha/alpha.cc
index 1126cea1f7ba2..e090e74b9d073 100644
--- a/gcc/config/alpha/alpha.cc
+++ b/gcc/config/alpha/alpha.cc
@@ -3820,6 +3820,12 @@  alpha_expand_block_move (rtx operands[])
           else if (a >= 16 && c % 2 == 0)
 	    src_align = 16;
 	}
+
+      if (MEM_P (orig_src) && MEM_ALIGN (orig_src) < src_align)
+	{
+	  orig_src = shallow_copy_rtx (orig_src);
+	  set_mem_align (orig_src, src_align);
+	}
     }
 
   tmp = XEXP (orig_dst, 0);
@@ -3841,6 +3847,12 @@  alpha_expand_block_move (rtx operands[])
           else if (a >= 16 && c % 2 == 0)
 	    dst_align = 16;
 	}
+
+      if (MEM_P (orig_dst) && MEM_ALIGN (orig_dst) < dst_align)
+	{
+	  orig_dst = shallow_copy_rtx (orig_dst);
+	  set_mem_align (orig_dst, dst_align);
+	}
     }
 
   ofs = 0;