[MSP430,PR78554] Prevent SUBREG from referencing a SYMBOL_REF

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
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

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

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