Re: [PATCH v2] [PR100106] Reject unaligned subregs when strict alignment is required

[Alexandre Oliva <oliva@adacore.com>]

On May  5, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> On Thu, May 05, 2022 at 03:52:01AM -0300, Alexandre Oliva wrote:
>> +  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?

I was going for conceptual grouping of alignment-related subexprs,
but I don't care enough to fight for it.

> Note that you should never have paradoxical subregs of mem on rs6000 or
> any other target with INSN_SCHEDULING.

Great, that alleviates some of my concerns about overreaching in this patch.

>> +#include "../../gcc.c-torture/compile/pr100106.c"

> It is better to copy the 11 lines of code.

'k

> Please comment what the ilp32 is for (namely, the -mcpu= will barf
> without it)..

Ack

> The testcase is okay with those changes, thanks!

Thanks.  Here's the revised patch.

I'm now testing on several platforms a follow-up patch that introduces
TARGET_ALLOW_SUBREG_OF_MEM.


[PR100106] Reject unaligned subregs when strict alignment is required

From: Alexandre Oliva <oliva@adacore.com>

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.


for  gcc/ChangeLog

	PR target/100106
	* emit-rtl.cc (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                                |    4 ++++
 gcc/testsuite/gcc.target/powerpc/pr100106-sa.c |   15 +++++++++++++++
 2 files changed, 19 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..9c03e27894fff 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -982,6 +982,10 @@ validate_subreg (machine_mode omode, machine_mode imode,
 
       return subreg_offset_representable_p (regno, imode, offset, omode);
     }
+  /* Do not allow SUBREG with stricter alignment than the inner MEM.  */
+  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 0000000000000..87634efa8d0b7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr100106-sa.c
@@ -0,0 +1,15 @@
+/* Require ilp32 because -mcpu=604 won't do 64 bits.  */
+/* { dg-do compile { target { ilp32 } } } */
+/* { dg-options "-mcpu=604 -O -mstrict-align" } */
+
+union a {
+  float _Complex b;
+  long long c;
+};
+
+void g(union a);
+
+void e() {
+  union a f = {1.0f};
+  g(f);
+}


-- 
Alexandre Oliva, happy hacker                https://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 <https://stallmansupport.org>