Patchwork [ARM] Fix Thumb-1 reload ICE with nested functions

login
register
mail settings
Submitter Julian Brown
Date Oct. 13, 2010, 12:47 p.m.
Message ID <20101013134756.193154f7@rex.config>
Download mbox | patch
Permalink /patch/67679/
State New
Headers show

Comments

Julian Brown - Oct. 13, 2010, 12:47 p.m.
Hi,

This patch fixes (fixed) several Fortran tests which ICE'd on 4.4
series compilers (with an "unable to find a register to spill in class
'HI_REGS'" error). Those same tests no longer fail with current
mainline and I don't have another test case which fails on mainline at
present, but I believe the patch still constitutes a correct change, so
should be applied. The problem is related to nested functions rather
than Fortran itself, though the testsuite for the latter happened to be
where the problem showed up.

The problem was originally described in the following mail (but has showed up independently in the meantime):

http://gcc.gnu.org/ml/gcc-patches/2008-04/msg02033.html

I've based the fix on Richard Sandiford's suggestion in the followup:

http://gcc.gnu.org/ml/gcc-patches/2008-05/msg00013.html

To recap (*): r12 is referenced as a hard register early in RTL generation,
in an instruction which requires reloading. High registers are marked
fixed, so are generally unavailable to reload -- but reload chooses to
attempt to use a high register for the reload anyway.

Now: the thumb1_movsi_insn pattern contains a constraint "..., *lhk".
The scope of the '*' modifier is just the following character: during
register preferencing, GCC chooses HI_REGS (h) for that alternative. I
believe the intention was for the '*' modifier to cover the whole
constraint, but this must be written "*l*h*k". This is the first part
of the fix.

(Other Thumb-1 instructions refer to "*lk", i.e. LO_REGS or the stack
pointer, but ignoring LO_REGS for preferencing purposes. Those might be
wrong too, but I've left them alone.)

The second problem is that only one register class is chosen for each
constraint when reloading. The "lhk" combination doesn't correspond to
any existing class, because the likely contender (CORE_REGS) also
contains the soft frame pointer. Richard S. claims that is unnecessary:
removing the soft frame pointer from the class doesn't seem to cause
any issues (at least for gcc, g++ & libstdc++ for -mthumb). If this
change is omitted, the ICE in the Fortran tests still occurs (HI_REGS
is still chosen for the reload).

Tested with cross to ARM Linux (gcc, g++, libstdc++, gfortran) with no
notable change in results. OK to apply?

Thanks,

Julian

(*) This description was written whilst I had a working test case for
the failure.

ChangeLog

    gcc/
    * config/arm/arm.h (REG_CLASS_CONTENTS): Remove soft frame pointer
    from CORE_REGS and GENERAL_REGS classes.
    * config/arm/arm.md (*thumb1_movsi_insn): Ignore all parts of final
    constraint for register preferencing.
Paul Brook - Oct. 13, 2010, 1:47 p.m.
>     gcc/
>     * config/arm/arm.h (REG_CLASS_CONTENTS): Remove soft frame pointer
>     from CORE_REGS and GENERAL_REGS classes.
>     * config/arm/arm.md (*thumb1_movsi_insn): Ignore all parts of final
>     constraint for register preferencing.

Ok.

For the record, I initially had concerns that this might prevent high 
registers being used at all (using stack spill slots instead). These concerns 
turned out to be unfounded.

Paul

Patch

Index: gcc/config/arm/arm.h
===================================================================
--- gcc/config/arm/arm.h	(revision 165177)
+++ gcc/config/arm/arm.h	(working copy)
@@ -1245,8 +1245,8 @@  enum reg_class
   { 0x0000DF00, 0x00000000, 0x00000000, 0x00000000 }, /* HI_REGS */	\
   { 0x01000000, 0x00000000, 0x00000000, 0x00000000 }, /* CC_REG */	\
   { 0x00000000, 0x00000000, 0x00000000, 0x80000000 }, /* VFPCC_REG */	\
-  { 0x0200DFFF, 0x00000000, 0x00000000, 0x00000000 }, /* GENERAL_REGS */ \
-  { 0x0200FFFF, 0x00000000, 0x00000000, 0x00000000 }, /* CORE_REGS */	\
+  { 0x0000DFFF, 0x00000000, 0x00000000, 0x00000000 }, /* GENERAL_REGS */ \
+  { 0x0000FFFF, 0x00000000, 0x00000000, 0x00000000 }, /* CORE_REGS */	\
   { 0xFAFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x7FFFFFFF }  /* ALL_REGS */	\
 }
 
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 165177)
+++ gcc/config/arm/arm.md	(working copy)
@@ -5163,8 +5163,8 @@ 
 )
 
 (define_insn "*thumb1_movsi_insn"
-  [(set (match_operand:SI 0 "nonimmediate_operand" "=l,l,l,l,l,>,l, m,*lhk")
-	(match_operand:SI 1 "general_operand"      "l, I,J,K,>,l,mi,l,*lhk"))]
+  [(set (match_operand:SI 0 "nonimmediate_operand" "=l,l,l,l,l,>,l, m,*l*h*k")
+	(match_operand:SI 1 "general_operand"      "l, I,J,K,>,l,mi,l,*l*h*k"))]
   "TARGET_THUMB1
    && (   register_operand (operands[0], SImode) 
        || register_operand (operands[1], SImode))"