Re: [PATCH] [PR100106] Reject unaligned subregs when strict alignment is required
On 2022-05-05 02:52, Alexandre Oliva wrote:
Regstrapped on x86_64-linux-gnu and ppc64le-linux-gnu, also tested
targeting ppc- and ppc64-vx7r2. Ok to install?
I am ok with the modified version of the patch. It looks reasonable for
me and I support its commit.
But I think I can not approve the patch formally as emit-rtl.cc is out
of my jurisdiction and validate_subreg is used in many places besides RA.
Sorry, Alex, some global reviewer should do this.
for gcc/ChangeLog
PR target/100106
* emit-rtl.c (validate_subreg): Reject a SUBREG of a MEM that
requires stricter alignment than MEM's.
for gcc/testsuite/ChangeLog
PR target/100106
* gcc.target/powerpc/pr100106-sa.c: New.
---
gcc/emit-rtl.cc|3 +++
gcc/testsuite/gcc.target/powerpc/pr100106-sa.c |4
2 files changed, 7 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index 1e02ae254d012..642e47eada0d7 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -982,6 +982,9 @@ validate_subreg (machine_mode omode, machine_mode imode,
return subreg_offset_representable_p (regno, imode, offset, omode);
}
+ else if (reg && MEM_P (reg)
+ && STRICT_ALIGNMENT && MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode))
+return false;
/* The outer size must be ordered wrt the register size, otherwise
we wouldn't know at compile time how many registers the outer
diff --git a/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
new file mode 100644
index 0..6cc29595c8b25
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
@@ -0,0 +1,4 @@
+/* { dg-do compile { target { ilp32 } } } */
+/* { dg-options "-mcpu=604 -O -mstrict-align" } */
+
+#include "../../gcc.c-torture/compile/pr100106.c"
Re: [PATCH] [PR100106] Reject unaligned subregs when strict alignment is required
On Thu, May 05, 2022 at 03:52:01AM -0300, Alexandre Oliva wrote:
> The testcase for pr100106, compiled with optimization for 32-bit
> powerpc -mcpu=604 with -mstrict-align expands the initialization of a
> union from a float _Complex value into a load from an SCmode
> constant pool entry, aligned to 4 bytes, into a DImode pseudo,
> requiring 8-byte alignment.
> + else if (reg && MEM_P (reg)
> +&& STRICT_ALIGNMENT && MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode))
> +return false;
Please fix the line breaks? Either do a break before every &&, or put
as many things as possible on one line?
Note that you should never have paradoxical subregs of mem on rs6000 or
any other target with INSN_SCHEDULING.
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
> @@ -0,0 +1,4 @@
> +/* { dg-do compile { target { ilp32 } } } */
> +/* { dg-options "-mcpu=604 -O -mstrict-align" } */
> +
> +#include "../../gcc.c-torture/compile/pr100106.c"
It is better to copy the 11 lines of code.
Please comment what the ilp32 is for (namely, the -mcpu= will barf
without it).. The testcase is okay with those changes, thanks!
Seghr
Re: [PATCH] [PR100106] Reject unaligned subregs when strict alignment is required
On Thu, May 05, 2022 at 08:59:21AM +0100, Richard Sandiford wrote: > Alexandre Oliva via Gcc-patches writes: > I know this is the best being the enemy of the good, but given > that we're at the start of stage 1, would it be feasible to try > to get rid of (subreg (mem)) altogether for GCC 13? Yes please! > We could do > it target-by-target, with a target macro (yes, macro :-)) that opts > in to keeping the existing behaviour. (subreg (mem)) would then be > unconditionally invalid when the macro isn't defined. (Even in > debug expressions, since those ought to narrow to a mem anyway.) Or we can simply threaten to drop all unconverted targets. That way at least there is a *chance* (a slim chance, but still) that the conversion will ever be finished. Paradoxical subregs of memory are already not allowed on targets with instruction scheduling, btw. Segher
Re: [PATCH] [PR100106] Reject unaligned subregs when strict alignment is required
Alexandre Oliva via Gcc-patches writes:
> The testcase for pr100106, compiled with optimization for 32-bit
> powerpc -mcpu=604 with -mstrict-align expands the initialization of a
> union from a float _Complex value into a load from an SCmode
> constant pool entry, aligned to 4 bytes, into a DImode pseudo,
> requiring 8-byte alignment.
>
> The patch that introduced the testcase modified simplify_subreg to
> avoid changing the MEM to outermode, but simplify_gen_subreg still
> creates a SUBREG or a MEM that would require stricter alignment than
> MEM's, and lra_constraints appears to get confused by that, repeatedly
> creating unsatisfiable reloads for the SUBREG until it exceeds the
> insn count.
>
> Avoiding the unaligned SUBREG, expand splits the DImode dest into
> SUBREGs and loads each SImode word of the constant pool with the
> proper alignment.
>
>
> At the time of posting this patch, it occurred to me that maybe the test
> should allow paradoxical subregs of mems, or even that non-paradoxical
> subregs of mems should be allowed to change to a mode with stricter
> alignment, and the register allocator should deal with that somehow.
> WDYT?
>
>
> Regstrapped on x86_64-linux-gnu and ppc64le-linux-gnu, also tested
> targeting ppc- and ppc64-vx7r2. Ok to install?
>
>
> for gcc/ChangeLog
>
> PR target/100106
> * emit-rtl.c (validate_subreg): Reject a SUBREG of a MEM that
> requires stricter alignment than MEM's.
I know this is the best being the enemy of the good, but given
that we're at the start of stage 1, would it be feasible to try
to get rid of (subreg (mem)) altogether for GCC 13? We could do
it target-by-target, with a target macro (yes, macro :-)) that opts
in to keeping the existing behaviour. (subreg (mem)) would then be
unconditionally invalid when the macro isn't defined. (Even in
debug expressions, since those ought to narrow to a mem anyway.)
Thanks,
Richard
> for gcc/testsuite/ChangeLog
>
> PR target/100106
> * gcc.target/powerpc/pr100106-sa.c: New.
> ---
> gcc/emit-rtl.cc|3 +++
> gcc/testsuite/gcc.target/powerpc/pr100106-sa.c |4
> 2 files changed, 7 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
>
> diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
> index 1e02ae254d012..642e47eada0d7 100644
> --- a/gcc/emit-rtl.cc
> +++ b/gcc/emit-rtl.cc
> @@ -982,6 +982,9 @@ validate_subreg (machine_mode omode, machine_mode imode,
>
>return subreg_offset_representable_p (regno, imode, offset, omode);
> }
> + else if (reg && MEM_P (reg)
> +&& STRICT_ALIGNMENT && MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode))
> +return false;
>
>/* The outer size must be ordered wrt the register size, otherwise
> we wouldn't know at compile time how many registers the outer
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
> b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
> new file mode 100644
> index 0..6cc29595c8b25
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
> @@ -0,0 +1,4 @@
> +/* { dg-do compile { target { ilp32 } } } */
> +/* { dg-options "-mcpu=604 -O -mstrict-align" } */
> +
> +#include "../../gcc.c-torture/compile/pr100106.c"
[PATCH] [PR100106] Reject unaligned subregs when strict alignment is required
The testcase for pr100106, compiled with optimization for 32-bit
powerpc -mcpu=604 with -mstrict-align expands the initialization of a
union from a float _Complex value into a load from an SCmode
constant pool entry, aligned to 4 bytes, into a DImode pseudo,
requiring 8-byte alignment.
The patch that introduced the testcase modified simplify_subreg to
avoid changing the MEM to outermode, but simplify_gen_subreg still
creates a SUBREG or a MEM that would require stricter alignment than
MEM's, and lra_constraints appears to get confused by that, repeatedly
creating unsatisfiable reloads for the SUBREG until it exceeds the
insn count.
Avoiding the unaligned SUBREG, expand splits the DImode dest into
SUBREGs and loads each SImode word of the constant pool with the
proper alignment.
At the time of posting this patch, it occurred to me that maybe the test
should allow paradoxical subregs of mems, or even that non-paradoxical
subregs of mems should be allowed to change to a mode with stricter
alignment, and the register allocator should deal with that somehow.
WDYT?
Regstrapped on x86_64-linux-gnu and ppc64le-linux-gnu, also tested
targeting ppc- and ppc64-vx7r2. Ok to install?
for gcc/ChangeLog
PR target/100106
* emit-rtl.c (validate_subreg): Reject a SUBREG of a MEM that
requires stricter alignment than MEM's.
for gcc/testsuite/ChangeLog
PR target/100106
* gcc.target/powerpc/pr100106-sa.c: New.
---
gcc/emit-rtl.cc|3 +++
gcc/testsuite/gcc.target/powerpc/pr100106-sa.c |4
2 files changed, 7 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index 1e02ae254d012..642e47eada0d7 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -982,6 +982,9 @@ validate_subreg (machine_mode omode, machine_mode imode,
return subreg_offset_representable_p (regno, imode, offset, omode);
}
+ else if (reg && MEM_P (reg)
+ && STRICT_ALIGNMENT && MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode))
+return false;
/* The outer size must be ordered wrt the register size, otherwise
we wouldn't know at compile time how many registers the outer
diff --git a/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
new file mode 100644
index 0..6cc29595c8b25
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
@@ -0,0 +1,4 @@
+/* { dg-do compile { target { ilp32 } } } */
+/* { dg-options "-mcpu=604 -O -mstrict-align" } */
+
+#include "../../gcc.c-torture/compile/pr100106.c"
--
Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
Free Software Activist GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts. Ask me about
