[PATCH] [MSP430] [PR78554] Prevent SUBREG from referencing a SYMBOL_REF

[PATCH] [MSP430] [PR78554] Prevent SUBREG from referencing a SYMBOL_REF

[Jozef Lawrynowicz]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

As reported in PR78554, attempting to store an __int20 address in memory
causes an ICE due to an invalid insn. This only occurs at optimisation
levels higher than -O0 because these optimisation levels pass
-ftree-ter, which causes the compiler to try and do the store in
one instruction.
The issue in the insn is that a SUBREG references a SYMBOL_REF.

I guess the compiler gets into this situation because it assumes that
it can execute a move instruction where both src and dst are in memory,
but this isn't possible with __int20.

The attached patch prevents a instance of SUBREG being created where the
subword is a SYMBOL_REF.

If the patch is acceptable, I would appreciate if someone could commit
it for me, as I do not have write access.

Thanks,
Jozef

[-- Attachment #2: 0001-MSP430-Prevent-SUBREG-from-referencing-a-SYMBOL_REF.patch --]
[-- Type: text/plain, Size: 1870 bytes --]

From 72a419c1f470254f06c59c7824ae27818a18b555 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@somniumtech.com>
Date: Thu, 24 Aug 2017 12:08:36 +0000
Subject: [PATCH] MSP430: Prevent SUBREG from referencing a SYMBOL_REF

2017-08-XX	Jozef Lawrynowicz	<jozef.l@somniumtech.com>

	PR target/78554
	* gcc/emit-rtl.c (operand_subword): Dont pass SYMBOL_REF operand to
	simplify_gen_subreg.
	
gcc/testsuite
2017-08-XX	Jozef Lawrynowicz	<jozef.l@somniumtech.com>

	PR target/78554
	* gcc.target/msp430/pr78554.c: New test.
---
 gcc/emit-rtl.c                            |  5 +++++
 gcc/testsuite/gcc.target/msp430/pr78554.c | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/msp430/pr78554.c

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index c1438d6..c655605 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -1596,6 +1596,11 @@ operand_subword (rtx op, unsigned int offset, int validate_address, machine_mode
       && (offset + 1) * UNITS_PER_WORD > GET_MODE_SIZE (mode))
     return const0_rtx;
 
+  /* TER can cause SYMBOL_REFs to arrive here.  Don't pass them to
+     simplify_gen_subreg.  */
+  if (SYMBOL_REF_P (op))
+    return 0;
+
   /* Form a new MEM at the requested address.  */
   if (MEM_P (op))
     {
diff --git a/gcc/testsuite/gcc.target/msp430/pr78554.c b/gcc/testsuite/gcc.target/msp430/pr78554.c
new file mode 100644
index 0000000..aca98cc
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/pr78554.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { "*-*-*" } { "-mcpu=msp430" "-msmall" } { "" } } */
+/* { dg-options "-O1 -mlarge -mcpu=msp430x" } */
+
+unsigned char test_val = 0;
+
+typedef __int20 unsigned reg_t;
+
+struct holds_reg
+{
+  reg_t r0;
+};
+
+struct holds_reg ex = { 0 };
+
+int main (void)
+{
+  ex.r0 = (reg_t)(&test_val);
+  return 0;
+}
-- 
1.8.3.1

Re: [PATCH] [MSP430] [PR78554] Prevent SUBREG from referencing a SYMBOL_REF

[Jeff Law]

On 08/24/2017 07:18 AM, Jozef Lawrynowicz wrote:
> As reported in PR78554, attempting to store an __int20 address in memory
> causes an ICE due to an invalid insn. This only occurs at optimisation
> levels higher than -O0 because these optimisation levels pass
> -ftree-ter, which causes the compiler to try and do the store in
> one instruction.
> The issue in the insn is that a SUBREG references a SYMBOL_REF.
> 
> I guess the compiler gets into this situation because it assumes that
> it can execute a move instruction where both src and dst are in memory,
> but this isn't possible with __int20.
GCC doesn't really make this assumption, but it does rely on the
target's movXX expanders to handle generating correct code for a
mem->mem move.

Commonly on risc targets what you'll see is a movXX expanders like this:

(define_expand "movsi"
  [(set (match_operand:SI 0 "nonimmediate_operand")
        (match_operand:SI 1 "general_operand"))]
  ""
{
  /* One of the ops has to be in a register.  */
  if (!register_operand (operand1, SImode)
      && !register_operand (operand0, SImode))
    operands[1] = force_reg (SImode, operand1);



> 
> The attached patch prevents a instance of SUBREG being created where the
> subword is a SYMBOL_REF.
> 
> If the patch is acceptable, I would appreciate if someone could commit
> it for me, as I do not have write access.
I was going to look deeper at this, but can't as the trunk currently
aborts in init_derived_machine_modes when compiling the testcase.
Presumably something about Richard S's work is busted when it comes to
dealing with partial-word stuff.

I'll ping Richard S. to take a looksie.

Jeff

Re: [PATCH] [MSP430] [PR78554] Prevent SUBREG from referencing a SYMBOL_REF

[Jeff Law]

On 08/24/2017 07:18 AM, Jozef Lawrynowicz wrote:
> As reported in PR78554, attempting to store an __int20 address in memory
> causes an ICE due to an invalid insn. This only occurs at optimisation
> levels higher than -O0 because these optimisation levels pass
> -ftree-ter, which causes the compiler to try and do the store in
> one instruction.
> The issue in the insn is that a SUBREG references a SYMBOL_REF.
> 
> I guess the compiler gets into this situation because it assumes that
> it can execute a move instruction where both src and dst are in memory,
> but this isn't possible with __int20.
> 
> The attached patch prevents a instance of SUBREG being created where the
> subword is a SYMBOL_REF.
> 
> If the patch is acceptable, I would appreciate if someone could commit
> it for me, as I do not have write access.
The compiler is asked to put a the address of test_val into a memory
location.

When in large mode the address of test_val is larger than a word.  ie,
it is PSI mode and the word size is HImode.

So naturally the generic parts of the compiler try to split that up into
word sized chunks.  That's probably not a good idea for partial modes,
regardless of the underlying object (ie, a SYMBOL_REF, MEM, REG, CONST,
whatever).

I really wonder if the fix here is to special case partial integer modes
in store_bit_field_1 to use the movpxx rather than trying to break them
down into word sized hunks.

I am pretty sure that special casing SYMBOL_REFs like this patch does
isn't right.

jeff