Patch Fix PR1936,24894,31644,31786 AVR target

Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

Anatoly, could please review and comit this solution if it meets your 
approval.

The attached patch addresses BASE_POINTER spill failure ICE  bugs:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19636
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31786
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24894
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31644

The patch has been tested against gcc head of 20080121 , but with latest 
avr target revision (as noted on diff listing)

All example test cases included in  bug reports  now pass compilation 
without failure. (whereas they all failed before at -O2)

Testsuite (execute) has been run and there were no regressions.

                === gcc Summary ===

# of expected passes            11799
# of unexpected failures        52
# of unresolved testcases       23
# of unsupported tests          682
/cygdrive/e/awhconf/gcc/xgcc  version 4.3.0 20080121 (experimental) (GCC)


The cause of  problem was trying to use base pointer register to reload 
an expression that got spilled out of a register - when the were no base 
pointers left available!
This typically occurs when r30,r31 - they only freely available base 
pointer - was used for function pointer or EEPROM addressing.

The fix is based around introducing r26,r27 as an additional 'emergency' 
base pointer register.

1)  The LEGITIMIZE_RELOAD_ADDRESS substitution is disabled. This allow 
gcc to explore alternatives (which often avoid need for base pointer!)
2)  'legitimate_address_p()'  now accepts R26 as base pointer. No new 
assembler patterns were needed as these already existed.

These two changes remove basic problem, but as R26 cannot be used as 
base pointer without extra instructions  code quality suffers. To 
prevent this the following changes were made to limit unnecessary usage 
of r26 as base pointer, while still retaining situations where r26,r27 
is a good solution.

3) Register allocation order (priority) is changed. R26,R27 is now 
demoted to the last "call used" pair. R30/31 is now higher priority and 
if available will be used as base pointer before R26/27. R28/R29 remains 
at  lower priority (since it is call saved). I also cleaned up declarations.
4) Small change made to 'avr_hard_regno_mode_ok' to exclude possibility 
that a large operand can span off the range of available registers (R31+)
5) Existing Q Constraint was used to for memory addressing using base 
pointer registers. This preferred category has been expanded to include 
pre_inc and post_dec of pointer registers - thus promoting  r26/r27 for 
this situation.
6) The general memory 'm' constraint  is where r26/27 can be still 
assigned as a base pointer. This has been demoted using '?m' to  give it 
a lower score than "Q" constraint. (do not use ! - this prevent it being 
used after reload)


In addition:

6) HI and SImode patterns have "Q" constraint added - this was missing 
before. If anything pointers are more useful on larger operands!
7)  'legitimate_address_p()' was rejecting  address registers that were 
contained inside subreg RTL patterns. e.g. (subreg:HI ((reg:SI 46) 0)). 
Such patterns are created when a structure is stored in register. If 
this structure contained pointer, it would not be recognized immediately 
as a valid address. e.g  same as (reg:HI 46). This led to poor pointer 
code often missing easy opportunities for post increment and base 
+offset instructions.  The simple solution (taken from MIPS port)  is to 
take the inner register.

With above changes, it as rare to see r26 used as base pointer - and 
beneficial when it does get used. Overall perhaps a small win on code 
size and speed. But importantly 4 less bugs!

Andy



Index: avr.c
===================================================================
--- avr.c    (revision 132049)
+++ avr.c    (working copy)
@@ -946,6 +946,8 @@
            reload_completed ? "(reload_completed)": "",
            reload_in_progress ? "(reload_in_progress)": "",
            reg_renumber ? "(reg_renumber)" : "");
+      if (!strict && GET_CODE (x) == SUBREG)
+        x = SUBREG_REG (x);          
       if (GET_CODE (x) == PLUS
       && REG_P (XEXP (x, 0))
       && GET_CODE (XEXP (x, 1)) == CONST_INT
@@ -971,6 +973,7 @@
       if (fit)
     {
       if (! strict
+          || REGNO (XEXP (x,0)) == REG_X
           || REGNO (XEXP (x,0)) == REG_Y
           || REGNO (XEXP (x,0)) == REG_Z)
         r = BASE_POINTER_REGS;
@@ -1938,7 +1941,7 @@
       /* This is a paranoid case. LEGITIMIZE_RELOAD_ADDRESS must exclude
          it but I have this situation with extremal
          optimization options.  */
-     
+
       *l = 4;
       if (reg_base == reg_dest)
         return (AS2 (adiw,r26,%o1)      CR_TAB
@@ -4821,43 +4824,9 @@
 order_regs_for_local_alloc (void)
 {
   unsigned int i;
-  static const int order_0[] = {
-    24,25,
-    18,19,
-    20,21,
-    22,23,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_1[] = {
-    18,19,
-    20,21,
-    22,23,
-    24,25,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_2[] = {
-    25,24,
-    23,22,
-    21,20,
-    19,18,
-    30,31,
-    26,27,
-    28,29,
-    17,16,
-    15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    1,0,
-    32,33,34,35
-  };
+  static const int order_0[] = REG_ALLOC_ORDER_0;
+  static const int order_1[] = REG_ALLOC_ORDER_1;
+  static const int order_2[] = REG_ALLOC_ORDER_2;
  
   const int *order = (TARGET_ORDER_1 ? order_1 :
               TARGET_ORDER_2 ? order_2 :
@@ -5471,6 +5440,14 @@
            || xx == arg_pointer_rtx)
     return 1;        /* XXX frame & arg pointer checks */
     }
+    else if  (GET_CODE (x) == PRE_DEC || GET_CODE (x) == POST_INC)
+    {
+      int regno = REGNO (XEXP (x, 0));
+      if (regno == REG_Z || regno == REG_Y || regno == REG_X)
+        return 1;
+    }
+   
+   
   return 0;
 }
 
@@ -5673,7 +5650,7 @@
     return 1;
 
   /* Modes larger than QImode occupy consecutive registers.  */
-  if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER)
+  if (regno <= (REG_Z + 1) && (regno + GET_MODE_SIZE (mode)) > (REG_Z + 2))
     return 0;
 
   /* All modes larger than QImode should start in an even register.  */
Index: avr.h
===================================================================
--- avr.h    (revision 132049)
+++ avr.h    (working copy)
@@ -198,19 +198,29 @@
     1,1,/*  STACK */                \
     1,1 /* arg pointer */  }
 
-#define REG_ALLOC_ORDER {            \
-    24,25,                    \
-    18,19,                    \
-    20,21,                    \
-    22,23,                    \
-    30,31,                    \
-    26,27,                    \
-    28,29,                    \
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,    \
-    0,1,                    \
-    32,33,34,35                    \
-    }
 
+#define REG_ALLOC_ORDER_0 {\
+    18,22,20,24,19,23,21,25,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }  
+ 
+#define REG_ALLOC_ORDER_1 {\
+    18,19,20,21,22,23,24,25,30,31,26,27,28,29,\
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }
+   
+#define REG_ALLOC_ORDER_2 { \
+    25,24,23,22,21,20,19,18,30,31,26,27,28,29,\
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    1,0,\
+    32,33,34,35  }
+
+
+#define REG_ALLOC_ORDER REG_ALLOC_ORDER_0
+
+
 #define ORDER_REGS_FOR_LOCAL_ALLOC order_regs_for_local_alloc ()
 
 
@@ -452,11 +462,14 @@
                    OPNUM, TYPE);                    \
           goto WIN;                                \
         }                                    \
+        if(0)                                   \
+        {                                       \
       push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,        \
                BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,        \
                OPNUM, TYPE);                        \
           goto WIN;                                \
     }                                    \
+    }                                    \
       else if (! (frame_pointer_needed && XEXP (X,0) == 
frame_pointer_rtx)) \
     {                                    \
       push_reload (X, NULL_RTX, &X, NULL,                    \
Index: avr.md
===================================================================
--- avr.md    (revision 132049)
+++ avr.md    (working copy)
@@ -169,8 +169,8 @@
   ")
 
 (define_insn "*movqi"
-  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,d,Qm,r,q,r,*r")
-    (match_operand:QI 1 "general_operand"       "r,i,rL,Qm,r,q,i"))]
+  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,d,Q?m,r,q,r,*r")
+    (match_operand:QI 1 "general_operand"       "r,i,rL,Q?m,r,q,i"))]
   "(register_operand (operands[0],QImode)
     || register_operand (operands[1], QImode) || const0_rtx == 
operands[1])"
   "* return output_movqi (insn, operands, NULL);"
@@ -250,8 +250,8 @@
    (set_attr "cc" "none")])
 
 (define_insn "*movhi"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,d,*r,q,r")
-        (match_operand:HI 1 "general_operand"       "r,m,rL,i,i,r,q"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,Q?m,d,*r,q,r")
+        (match_operand:HI 1 "general_operand"       "r,Q?m,rL,i,i,r,q"))]
   "(register_operand (operands[0],HImode)
     || register_operand (operands[1],HImode) || const0_rtx == operands[1])"
   "* return output_movhi (insn, operands, NULL);"
@@ -328,8 +328,8 @@
 
 
 (define_insn "*movsi"
-  [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,Qm,!d,r")
-        (match_operand:SI 1 "general_operand"       "r,L,Qm,rL,i,i"))]
+  [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,Q?m,!d,r")
+        (match_operand:SI 1 "general_operand"       "r,L,Q?m,rL,i,i"))]
   "(register_operand (operands[0],SImode)
     || register_operand (operands[1],SImode) || const0_rtx == operands[1])"
   "* return output_movsisf (insn, operands, NULL);"

Re: Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

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

Patch is updated to leave  register allocation same as HEAD for default 
ordering

I have made -morder2  new alternative - ( the previous order2 always 
created worse code so it's loss is unlikely to upset anyone.). We can 
consider using this as default at a latter date.

In addition, I added "Q" constraint to HI mode moves. This will  prefer 
HI mode use of  pointer registers X,Y,Z  - but not when X is used as 
base pointer.  QI, SI and SF mode already used this constraint and are 
unchanged.

Tested with no regression.

If this is acceptable, please comit.

Andy


Andrew Hutchinson wrote:
> Anatoly, could please review and comit this solution if it meets your 
> approval.
>
> The attached patch addresses BASE_POINTER spill failure ICE  bugs:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19636
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31786
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24894
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31644
>
> The patch has been tested against gcc head of 20080121 , but with 
> latest avr target revision (as noted on diff listing)
>
> All example test cases included in  bug reports  now pass compilation 
> without failure. (whereas they all failed before at -O2)
>
> Testsuite (execute) has been run and there were no regressions.
>
>                === gcc Summary ===
>
> # of expected passes            11799
> # of unexpected failures        52
> # of unresolved testcases       23
> # of unsupported tests          682
> /cygdrive/e/awhconf/gcc/xgcc  version 4.3.0 20080121 (experimental) (GCC)
>
>
> The cause of  problem was trying to use base pointer register to 
> reload an expression that got spilled out of a register - when the 
> were no base pointers left available!
> This typically occurs when r30,r31 - they only freely available base 
> pointer - was used for function pointer or EEPROM addressing.
>
> The fix is based around introducing r26,r27 as an additional 
> 'emergency' base pointer register.
>
> 1)  The LEGITIMIZE_RELOAD_ADDRESS substitution is disabled. This allow 
> gcc to explore alternatives (which often avoid need for base pointer!)
> 2)  'legitimate_address_p()'  now accepts R26 as base pointer. No new 
> assembler patterns were needed as these already existed.
>
> These two changes remove basic problem, but as R26 cannot be used as 
> base pointer without extra instructions  code quality suffers. To 
> prevent this the following changes were made to limit unnecessary 
> usage of r26 as base pointer, while still retaining situations where 
> r26,r27 is a good solution.
>
> 3) Register allocation order (priority) is changed. R26,R27 is now 
> demoted to the last "call used" pair. R30/31 is now higher priority 
> and if available will be used as base pointer before R26/27. R28/R29 
> remains at  lower priority (since it is call saved). I also cleaned up 
> declarations.
> 4) Small change made to 'avr_hard_regno_mode_ok' to exclude 
> possibility that a large operand can span off the range of available 
> registers (R31+)
> 5) Existing Q Constraint was used to for memory addressing using base 
> pointer registers. This preferred category has been expanded to 
> include pre_inc and post_dec of pointer registers - thus promoting  
> r26/r27 for this situation.
> 6) The general memory 'm' constraint  is where r26/27 can be still 
> assigned as a base pointer. This has been demoted using '?m' to  give 
> it a lower score than "Q" constraint. (do not use ! - this prevent it 
> being used after reload)
>
>
> In addition:
>
> 6) HI and SImode patterns have "Q" constraint added - this was missing 
> before. If anything pointers are more useful on larger operands!
> 7)  'legitimate_address_p()' was rejecting  address registers that 
> were contained inside subreg RTL patterns. e.g. (subreg:HI ((reg:SI 
> 46) 0)). Such patterns are created when a structure is stored in 
> register. If this structure contained pointer, it would not be 
> recognized immediately as a valid address. e.g  same as (reg:HI 46). 
> This led to poor pointer code often missing easy opportunities for 
> post increment and base +offset instructions.  The simple solution 
> (taken from MIPS port)  is to take the inner register.
>
> With above changes, it as rare to see r26 used as base pointer - and 
> beneficial when it does get used. Overall perhaps a small win on code 
> size and speed. But importantly 4 less bugs!
>
> Andy
>
>

[-- Attachment #2: spill.diff --]
[-- Type: text/plain, Size: 4895 bytes --]

Index: avr.c
===================================================================
--- avr.c	(revision 132366)
+++ avr.c	(working copy)
@@ -965,6 +965,8 @@
 	       reload_completed ? "(reload_completed)": "",
 	       reload_in_progress ? "(reload_in_progress)": "",
 	       reg_renumber ? "(reg_renumber)" : "");
+      if (!strict && GET_CODE (x) == SUBREG)
+	    x = SUBREG_REG (x);		   
       if (GET_CODE (x) == PLUS
 	  && REG_P (XEXP (x, 0))
 	  && GET_CODE (XEXP (x, 1)) == CONST_INT
@@ -990,6 +992,7 @@
       if (fit)
 	{
 	  if (! strict
+	      || REGNO (XEXP (x,0)) == REG_X
 	      || REGNO (XEXP (x,0)) == REG_Y
 	      || REGNO (XEXP (x,0)) == REG_Z)
 	    r = BASE_POINTER_REGS;
@@ -1957,7 +1960,7 @@
 	  /* This is a paranoid case. LEGITIMIZE_RELOAD_ADDRESS must exclude
 	     it but I have this situation with extremal
 	     optimization options.  */
-	  
+
 	  *l = 4;
 	  if (reg_base == reg_dest)
 	    return (AS2 (adiw,r26,%o1)      CR_TAB
@@ -4840,43 +4843,9 @@
 order_regs_for_local_alloc (void)
 {
   unsigned int i;
-  static const int order_0[] = {
-    24,25,
-    18,19,
-    20,21,
-    22,23,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_1[] = {
-    18,19,
-    20,21,
-    22,23,
-    24,25,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_2[] = {
-    25,24,
-    23,22,
-    21,20,
-    19,18,
-    30,31,
-    26,27,
-    28,29,
-    17,16,
-    15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    1,0,
-    32,33,34,35
-  };
+  static const int order_0[] = REG_ALLOC_ORDER_0;
+  static const int order_1[] = REG_ALLOC_ORDER_1;
+  static const int order_2[] = REG_ALLOC_ORDER_2;
   
   const int *order = (TARGET_ORDER_1 ? order_1 :
 		      TARGET_ORDER_2 ? order_2 :
@@ -5490,6 +5459,14 @@
 	       || xx == arg_pointer_rtx)
 	return 1;		/* XXX frame & arg pointer checks */
     }
+	else if  (GET_CODE (x) == PRE_DEC || GET_CODE (x) == POST_INC)
+	{
+	  int regno = REGNO (XEXP (x, 0));
+	  if (regno == REG_Z || regno == REG_Y || regno == REG_X)
+	    return 1;
+	}
+	
+	
   return 0;
 }
 
@@ -5692,7 +5669,7 @@
     return 1;
 
   /* Modes larger than QImode occupy consecutive registers.  */
-  if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER)
+  if (regno <= (REG_Z + 1) && (regno + GET_MODE_SIZE (mode)) > (REG_Z + 2))
     return 0;
 
   /* All modes larger than QImode should start in an even register.  */
Index: avr.h
===================================================================
--- avr.h	(revision 132366)
+++ avr.h	(working copy)
@@ -199,19 +199,29 @@
     1,1,/*  STACK */				\
     1,1 /* arg pointer */  }
 
-#define REG_ALLOC_ORDER {			\
-    24,25,					\
-    18,19,					\
-    20,21,					\
-    22,23,					\
-    30,31,					\
-    26,27,					\
-    28,29,					\
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,	\
-    0,1,					\
-    32,33,34,35					\
-    }
 
+#define REG_ALLOC_ORDER_0 {\
+    24,25,18,19,20,21,22,23,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }   
+  
+#define REG_ALLOC_ORDER_1 {\
+    18,19,20,21,22,23,24,25,30,31,26,27,28,29,\
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }
+
+#define REG_ALLOC_ORDER_2 {\
+    18,22,20,24,19,23,21,25,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 } 
+
+
+#define REG_ALLOC_ORDER REG_ALLOC_ORDER_0
+
+
 #define ORDER_REGS_FOR_LOCAL_ALLOC order_regs_for_local_alloc ()
 
 
@@ -453,11 +463,14 @@
 		           OPNUM, TYPE);				    \
 	      goto WIN;							    \
 	    }								    \
+        if(0)                                   \
+        {                                       \
 	  push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,	    \
 		       BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,	    \
 		       OPNUM, TYPE);					    \
           goto WIN;							    \
 	}								    \
+	}								    \
       else if (! (frame_pointer_needed && XEXP (X,0) == frame_pointer_rtx)) \
 	{								    \
 	  push_reload (X, NULL_RTX, &X, NULL,				    \
Index: avr.md
===================================================================
--- avr.md	(revision 132366)
+++ avr.md	(working copy)
@@ -251,8 +251,8 @@
    (set_attr "cc" "none")])
 
 (define_insn "*movhi"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,d,*r,q,r")
-        (match_operand:HI 1 "general_operand"       "r,m,rL,i,i,r,q"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,Qm,d,*r,q,r")
+        (match_operand:HI 1 "general_operand"       "r,Qm,rL,i,i,r,q"))]
   "(register_operand (operands[0],HImode)
     || register_operand (operands[1],HImode) || const0_rtx == operands[1])"
   "* return output_movhi (insn, operands, NULL);"

Patch Fix PR35013, PR27192

[Andrew Hutchinson]

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

The attached patch fixes

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35013
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27192

Both of these problems occur because the avr backend fails to recognize 
function pointer expressions - such as foo + 2

As a result, these references are treated as byte addresses by 
assembler, rather word addresses using the pm() directive. The patch 
changes the operand check to one that finds the function reference 
buried in the expression. (This is similar to code used on the SPARC-gcc)

This patch has been tested using gcc version 4.3.0 20080121 
(experimental)  with HEAD versions of the AVR files. No regressions 
occurred running gcc.c-torture/execute testsuite with simulator and the 
PR test cases generate the correct code.

Please comit.

2008-02-16 Andy Hutchinson <hutchinsonandy@aim.com>

    PR avr/35013 avr/27192

    * avr.h (text_segment_operand): New function to check program memory 
address.

    * avr.c (text_segment_operand): New function to check program memory 
address.
    (print_operand_address,  avr_assemble_integer): Add new check.






[-- Attachment #2: funcarith.diff --]
[-- Type: text/plain, Size: 2131 bytes --]

Index: avr-protos.h
===================================================================
--- avr-protos.h	(revision 132369)
+++ avr-protos.h	(working copy)
@@ -111,6 +111,7 @@
 extern int _reg_unused_after (rtx insn, rtx reg);
 extern int avr_jump_mode (rtx x, rtx insn);
 extern int byte_immediate_operand (rtx op, enum machine_mode mode);
+extern int text_segment_operand (rtx op, enum machine_mode mode);
 extern int test_hard_reg_class (enum reg_class class, rtx x);
 extern int jump_over_one_insn_p (rtx insn, rtx dest);
 
Index: avr.c
===================================================================
--- avr.c	(revision 132366)
+++ avr.c	(working copy)
@@ -1116,8 +1116,7 @@
 
     default:
       if (CONSTANT_ADDRESS_P (addr)
-	  && ((GET_CODE (addr) == SYMBOL_REF && SYMBOL_REF_FUNCTION_P (addr))
-	      || GET_CODE (addr) == LABEL_REF))
+	  && text_segment_operand (addr, VOIDmode))
 	{
 	  fprintf (file, "pm(");
 	  output_addr_const (file,addr);
@@ -1428,6 +1427,26 @@
           && INTVAL (op) <= 0xff && INTVAL (op) >= 0);
 }
 
+/* Return true if OP is a program memory reference.*/
+int 
+text_segment_operand (rtx op, enum machine_mode mode ATTRIBUTE_UNUSED)
+{
+  switch (GET_CODE (op))
+    {
+    case LABEL_REF :
+      return true;
+    case SYMBOL_REF :
+      return SYMBOL_REF_FUNCTION_P (op);
+    case PLUS :
+      /* Assume canonical format of symbol + constant.
+	 Fall through.  */
+    case CONST :
+      return text_segment_operand (XEXP (op, 0), VOIDmode);
+    default :
+      return false;
+    }
+}
+
 /* Output all insn addresses and their sizes into the assembly language
    output file.  This is helpful for debugging whether the length attributes
    in the md file are correct.
@@ -4465,8 +4484,7 @@
 avr_assemble_integer (rtx x, unsigned int size, int aligned_p)
 {
   if (size == POINTER_SIZE / BITS_PER_UNIT && aligned_p
-      && ((GET_CODE (x) == SYMBOL_REF && SYMBOL_REF_FUNCTION_P (x))
-	  || GET_CODE (x) == LABEL_REF))
+      && text_segment_operand (x, VOIDmode) )
     {
       fputs ("\t.word\tpm(", asm_out_file);
       output_addr_const (asm_out_file, x);

Re: Patch Fix PR35013, PR27192

[Andrew Hutchinson]

Slight mistake in change info:

PR avr/35013 avr/27192

* avr-protos.h(text_segment_operand): New function to check program 
memory address.

* avr.c (text_segment_operand): New function to check program memory 
address.
(print_operand_address, avr_assemble_integer): Add new check.

RE: Patch Fix PR1936,24894,31644,31786 AVR target

[Weddington, Eric]

Hi Andy,

I couldn't apply your patch to HEAD. It wouldn't patch against avr.md.
Could you take a look at this again?

Thanks,
Eric Weddington 

> -----Original Message-----
> From: Andrew Hutchinson [mailto:andrewhutchinson@cox.net] 
> Sent: Saturday, February 16, 2008 12:47 PM
> To: Anatoly Sokolov; gcc-patches@gcc.gnu.org; Weddington, Eric
> Subject: Re: Patch Fix PR1936,24894,31644,31786 AVR target
> 
> Patch is updated to leave  register allocation same as HEAD 
> for default 
> ordering
> 
> I have made -morder2  new alternative - ( the previous order2 always 
> created worse code so it's loss is unlikely to upset anyone.). We can 
> consider using this as default at a latter date.
> 
> In addition, I added "Q" constraint to HI mode moves. This 
> will  prefer 
> HI mode use of  pointer registers X,Y,Z  - but not when X is used as 
> base pointer.  QI, SI and SF mode already used this 
> constraint and are 
> unchanged.
> 
> Tested with no regression.
> 
> If this is acceptable, please comit.
> 
> Andy
> 
> 
> Andrew Hutchinson wrote:
> > Anatoly, could please review and comit this solution if it 
> meets your 
> > approval.
> >
> > The attached patch addresses BASE_POINTER spill failure ICE  bugs:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19636
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31786
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24894
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31644
> >
> > The patch has been tested against gcc head of 20080121 , but with 
> > latest avr target revision (as noted on diff listing)
> >
> > All example test cases included in  bug reports  now pass 
> compilation 
> > without failure. (whereas they all failed before at -O2)
> >
> > Testsuite (execute) has been run and there were no regressions.
> >
> >                === gcc Summary ===
> >
> > # of expected passes            11799
> > # of unexpected failures        52
> > # of unresolved testcases       23
> > # of unsupported tests          682
> > /cygdrive/e/awhconf/gcc/xgcc  version 4.3.0 20080121 
> (experimental) (GCC)
> >
> >
> > The cause of  problem was trying to use base pointer register to 
> > reload an expression that got spilled out of a register - when the 
> > were no base pointers left available!
> > This typically occurs when r30,r31 - they only freely 
> available base 
> > pointer - was used for function pointer or EEPROM addressing.
> >
> > The fix is based around introducing r26,r27 as an additional 
> > 'emergency' base pointer register.
> >
> > 1)  The LEGITIMIZE_RELOAD_ADDRESS substitution is disabled. 
> This allow 
> > gcc to explore alternatives (which often avoid need for 
> base pointer!)
> > 2)  'legitimate_address_p()'  now accepts R26 as base 
> pointer. No new 
> > assembler patterns were needed as these already existed.
> >
> > These two changes remove basic problem, but as R26 cannot 
> be used as 
> > base pointer without extra instructions  code quality suffers. To 
> > prevent this the following changes were made to limit unnecessary 
> > usage of r26 as base pointer, while still retaining 
> situations where 
> > r26,r27 is a good solution.
> >
> > 3) Register allocation order (priority) is changed. R26,R27 is now 
> > demoted to the last "call used" pair. R30/31 is now higher priority 
> > and if available will be used as base pointer before 
> R26/27. R28/R29 
> > remains at  lower priority (since it is call saved). I also 
> cleaned up 
> > declarations.
> > 4) Small change made to 'avr_hard_regno_mode_ok' to exclude 
> > possibility that a large operand can span off the range of 
> available 
> > registers (R31+)
> > 5) Existing Q Constraint was used to for memory addressing 
> using base 
> > pointer registers. This preferred category has been expanded to 
> > include pre_inc and post_dec of pointer registers - thus promoting  
> > r26/r27 for this situation.
> > 6) The general memory 'm' constraint  is where r26/27 can be still 
> > assigned as a base pointer. This has been demoted using 
> '?m' to  give 
> > it a lower score than "Q" constraint. (do not use ! - this 
> prevent it 
> > being used after reload)
> >
> >
> > In addition:
> >
> > 6) HI and SImode patterns have "Q" constraint added - this 
> was missing 
> > before. If anything pointers are more useful on larger operands!
> > 7)  'legitimate_address_p()' was rejecting  address registers that 
> > were contained inside subreg RTL patterns. e.g. (subreg:HI ((reg:SI 
> > 46) 0)). Such patterns are created when a structure is stored in 
> > register. If this structure contained pointer, it would not be 
> > recognized immediately as a valid address. e.g  same as 
> (reg:HI 46). 
> > This led to poor pointer code often missing easy opportunities for 
> > post increment and base +offset instructions.  The simple solution 
> > (taken from MIPS port)  is to take the inner register.
> >
> > With above changes, it as rare to see r26 used as base 
> pointer - and 
> > beneficial when it does get used. Overall perhaps a small 
> win on code 
> > size and speed. But importantly 4 less bugs!
> >
> > Andy
> >
> >
> 

Re: Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

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

Oddly, I had no problems. But I re-created it just the same:

The change to avr.md is only one line. I'm guessing there was conflict 
with recent RAMPZ change.?

Andy




Try attached.


Weddington, Eric wrote:
> Hi Andy,
>
> I couldn't apply your patch to HEAD. It wouldn't patch against avr.md.
> Could you take a look at this again?
>
> Thanks,
> Eric Weddington 
>   
>

[-- Attachment #2: spill2.diff --]
[-- Type: text/plain, Size: 4895 bytes --]

Index: avr.c
===================================================================
--- avr.c	(revision 132380)
+++ avr.c	(working copy)
@@ -965,6 +965,8 @@
 	       reload_completed ? "(reload_completed)": "",
 	       reload_in_progress ? "(reload_in_progress)": "",
 	       reg_renumber ? "(reg_renumber)" : "");
+      if (!strict && GET_CODE (x) == SUBREG)
+	    x = SUBREG_REG (x);		   
       if (GET_CODE (x) == PLUS
 	  && REG_P (XEXP (x, 0))
 	  && GET_CODE (XEXP (x, 1)) == CONST_INT
@@ -990,6 +992,7 @@
       if (fit)
 	{
 	  if (! strict
+	      || REGNO (XEXP (x,0)) == REG_X
 	      || REGNO (XEXP (x,0)) == REG_Y
 	      || REGNO (XEXP (x,0)) == REG_Z)
 	    r = BASE_POINTER_REGS;
@@ -1957,7 +1960,7 @@
 	  /* This is a paranoid case. LEGITIMIZE_RELOAD_ADDRESS must exclude
 	     it but I have this situation with extremal
 	     optimization options.  */
-	  
+
 	  *l = 4;
 	  if (reg_base == reg_dest)
 	    return (AS2 (adiw,r26,%o1)      CR_TAB
@@ -4840,43 +4843,9 @@
 order_regs_for_local_alloc (void)
 {
   unsigned int i;
-  static const int order_0[] = {
-    24,25,
-    18,19,
-    20,21,
-    22,23,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_1[] = {
-    18,19,
-    20,21,
-    22,23,
-    24,25,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_2[] = {
-    25,24,
-    23,22,
-    21,20,
-    19,18,
-    30,31,
-    26,27,
-    28,29,
-    17,16,
-    15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    1,0,
-    32,33,34,35
-  };
+  static const int order_0[] = REG_ALLOC_ORDER_0;
+  static const int order_1[] = REG_ALLOC_ORDER_1;
+  static const int order_2[] = REG_ALLOC_ORDER_2;
   
   const int *order = (TARGET_ORDER_1 ? order_1 :
 		      TARGET_ORDER_2 ? order_2 :
@@ -5490,6 +5459,14 @@
 	       || xx == arg_pointer_rtx)
 	return 1;		/* XXX frame & arg pointer checks */
     }
+	else if  (GET_CODE (x) == PRE_DEC || GET_CODE (x) == POST_INC)
+	{
+	  int regno = REGNO (XEXP (x, 0));
+	  if (regno == REG_Z || regno == REG_Y || regno == REG_X)
+	    return 1;
+	}
+	
+	
   return 0;
 }
 
@@ -5692,7 +5669,7 @@
     return 1;
 
   /* Modes larger than QImode occupy consecutive registers.  */
-  if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER)
+  if (regno <= (REG_Z + 1) && (regno + GET_MODE_SIZE (mode)) > (REG_Z + 2))
     return 0;
 
   /* All modes larger than QImode should start in an even register.  */
Index: avr.h
===================================================================
--- avr.h	(revision 132380)
+++ avr.h	(working copy)
@@ -199,19 +199,29 @@
     1,1,/*  STACK */				\
     1,1 /* arg pointer */  }
 
-#define REG_ALLOC_ORDER {			\
-    24,25,					\
-    18,19,					\
-    20,21,					\
-    22,23,					\
-    30,31,					\
-    26,27,					\
-    28,29,					\
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,	\
-    0,1,					\
-    32,33,34,35					\
-    }
 
+#define REG_ALLOC_ORDER_0 {\
+    24,25,18,19,20,21,22,23,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }   
+  
+#define REG_ALLOC_ORDER_1 {\
+    18,19,20,21,22,23,24,25,30,31,26,27,28,29,\
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }
+
+#define REG_ALLOC_ORDER_2 {\
+    18,22,20,24,19,23,21,25,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 } 
+
+
+#define REG_ALLOC_ORDER REG_ALLOC_ORDER_0
+
+
 #define ORDER_REGS_FOR_LOCAL_ALLOC order_regs_for_local_alloc ()
 
 
@@ -453,11 +463,14 @@
 		           OPNUM, TYPE);				    \
 	      goto WIN;							    \
 	    }								    \
+        if(0)                                   \
+        {                                       \
 	  push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,	    \
 		       BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,	    \
 		       OPNUM, TYPE);					    \
           goto WIN;							    \
 	}								    \
+	}								    \
       else if (! (frame_pointer_needed && XEXP (X,0) == frame_pointer_rtx)) \
 	{								    \
 	  push_reload (X, NULL_RTX, &X, NULL,				    \
Index: avr.md
===================================================================
--- avr.md	(revision 132380)
+++ avr.md	(working copy)
@@ -251,8 +251,8 @@
    (set_attr "cc" "none")])
 
 (define_insn "*movhi"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,d,*r,q,r")
-        (match_operand:HI 1 "general_operand"       "r,m,rL,i,i,r,q"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,Qm,d,*r,q,r")
+        (match_operand:HI 1 "general_operand"       "r,Qm,rL,i,i,r,q"))]
   "(register_operand (operands[0],HImode)
     || register_operand (operands[1],HImode) || const0_rtx == operands[1])"
   "* return output_movhi (insn, operands, NULL);"

RE: Patch Fix PR1936,24894,31644,31786 AVR target

[Weddington, Eric]

 

> -----Original Message-----
> From: Andrew Hutchinson [mailto:andrewhutchinson@cox.net] 
> Sent: Sunday, February 17, 2008 11:59 AM
> To: Weddington, Eric
> Cc: Anatoly Sokolov; gcc-patches@gcc.gnu.org
> Subject: Re: Patch Fix PR1936,24894,31644,31786 AVR target
> 
> Oddly, I had no problems. But I re-created it just the same:
> 
> The change to avr.md is only one line. I'm guessing there was 
> conflict 
> with recent RAMPZ change.?
> 
> Andy
> 
> 
> 
> 
> Try attached.


Ah, I found the problem. There is a conflict with the patch for bug
#11180:
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=11180>
The patch there turns *movhi from define_insn to define_insn_and_split,
which conflicts with your patch.

Andy, you had a comment to that bug report. Can you help resolve any
conflicts with these patches?

Thanks,
Eric Weddington

Re: Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

I would not recommend splitting the moves until the full impact has been 
looked  at.

There are some movhi (and larger) patterns that cannot be replicated by 
single byte moves.
For example:
If a base pointer needs more than 64 bytes displacement it will force 
use of inline addition and subtraction. (The subtraction is sometimes 
removed by mov pattern)
 If you split pattern, then that will create pointer addition and 
subtraction per byte of the move. From what I have seen, this is too 
late for gcc to further optimise

There may be other situations where the split does not work well or 
causes regression Volatiles, overlaps and the like are possible areas to 
look at.

In summary, the principle is fine but it needs careful testing. I 
suspect we will need to fix some other problems  before we can make this 
split effective.


FWIW I have created splitters for logical operations. These work without 
too much problem.
However, even here I have come across issues that cause simplistic 
splitter to be "imperfect". For example, creating ORI R16,#0. All 
fixable, but not quite as simple as may first appear.
At this very moment I'm chasing  down the abilities of gcc to optimise 
code RTL - post split. This is very relevant to the gains that might be 
expected.


Andy








Weddington, Eric wrote:
>  
>
>   
>> -----Original Message-----
>> From: Andrew Hutchinson [mailto:andrewhutchinson@cox.net] 
>> Sent: Sunday, February 17, 2008 11:59 AM
>> To: Weddington, Eric
>> Cc: Anatoly Sokolov; gcc-patches@gcc.gnu.org
>> Subject: Re: Patch Fix PR1936,24894,31644,31786 AVR target
>>
>> Oddly, I had no problems. But I re-created it just the same:
>>
>> The change to avr.md is only one line. I'm guessing there was 
>> conflict 
>> with recent RAMPZ change.?
>>
>> Andy
>>
>>
>>
>>
>> Try attached.
>>     
>
>
> Ah, I found the problem. There is a conflict with the patch for bug
> #11180:
> <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=11180>
> The patch there turns *movhi from define_insn to define_insn_and_split,
> which conflicts with your patch.
>
> Andy, you had a comment to that bug report. Can you help resolve any
> conflicts with these patches?
>
> Thanks,
> Eric Weddington
>
>   

Re: Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

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

I sent you this patch to approve and comit on 2008-02-2.

It passed execute tests no regressions.

Can it now be applied?




Andrew Hutchinson wrote:
> 2008-02-20  Andrew Hutchinson <hutchinsonandy@aim.com>
>
>    PR target/19636,24894,31644,31786
>     * config/avr/avr.h (REG_ALLOC_ORDER_2):    Move defintion from
>    avr.c. Re-order alternative 2.
>    (LEGITIMIZE_RELOAD_ADDRESS): Don't push base pointer as reload for
>    base pointer spill.
>    * config/avr/avr.c (legitimate_address_p): Expose address inside
>    subreg. Permit REG_X as base pointer.
>    (order_regs_for_local_alloc) Move constant definitions to avr.h.
>    (extra_constraint_Q): Add post-inc, pre-dec of X,Y Z to constraint.
>    * config/avr/avr.md (*movhi) Add Q constraint before m.
>
>
> I am re-testing modified patch. (It takes a few hours.)
>
>
>

[-- Attachment #2: spill3.diff --]
[-- Type: text/plain, Size: 4824 bytes --]

Index: avr.c
===================================================================
--- avr.c	(revision 132380)
+++ avr.c	(working copy)
@@ -976,6 +976,8 @@
 		 true_regnum (XEXP (x, 0)));
       debug_rtx (x);
     }
+  if (!strict && GET_CODE (x) == SUBREG)
+	x = SUBREG_REG (x);		   
   if (REG_P (x) && (strict ? REG_OK_FOR_BASE_STRICT_P (x)
                     : REG_OK_FOR_BASE_NOSTRICT_P (x)))
     r = POINTER_REGS;
@@ -990,6 +992,7 @@
       if (fit)
 	{
 	  if (! strict
+	      || REGNO (XEXP (x,0)) == REG_X
 	      || REGNO (XEXP (x,0)) == REG_Y
 	      || REGNO (XEXP (x,0)) == REG_Z)
 	    r = BASE_POINTER_REGS;
@@ -1957,7 +1960,7 @@
 	  /* This is a paranoid case. LEGITIMIZE_RELOAD_ADDRESS must exclude
 	     it but I have this situation with extremal
 	     optimization options.  */
-	  
+
 	  *l = 4;
 	  if (reg_base == reg_dest)
 	    return (AS2 (adiw,r26,%o1)      CR_TAB
@@ -4840,43 +4843,9 @@
 order_regs_for_local_alloc (void)
 {
   unsigned int i;
-  static const int order_0[] = {
-    24,25,
-    18,19,
-    20,21,
-    22,23,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_1[] = {
-    18,19,
-    20,21,
-    22,23,
-    24,25,
-    30,31,
-    26,27,
-    28,29,
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    0,1,
-    32,33,34,35
-  };
-  static const int order_2[] = {
-    25,24,
-    23,22,
-    21,20,
-    19,18,
-    30,31,
-    26,27,
-    28,29,
-    17,16,
-    15,14,13,12,11,10,9,8,7,6,5,4,3,2,
-    1,0,
-    32,33,34,35
-  };
+  static const int order_0[] = REG_ALLOC_ORDER_0;
+  static const int order_1[] = REG_ALLOC_ORDER_1;
+  static const int order_2[] = REG_ALLOC_ORDER_2;
   
   const int *order = (TARGET_ORDER_1 ? order_1 :
 		      TARGET_ORDER_2 ? order_2 :
@@ -5490,6 +5459,14 @@
 	       || xx == arg_pointer_rtx)
 	return 1;		/* XXX frame & arg pointer checks */
     }
+	else if  (GET_CODE (x) == PRE_DEC || GET_CODE (x) == POST_INC)
+	{
+	  int regno = REGNO (XEXP (x, 0));
+	  if (regno == REG_Z || regno == REG_Y || regno == REG_X)
+	    return 1;
+	}
+	
+	
   return 0;
 }
 
@@ -5692,7 +5669,7 @@
     return 1;
 
   /* Modes larger than QImode occupy consecutive registers.  */
-  if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER)
+  if (regno <= (REG_Z + 1) && (regno + GET_MODE_SIZE (mode)) > (REG_Z + 2))
     return 0;
 
   /* All modes larger than QImode should start in an even register.  */
Index: avr.h
===================================================================
--- avr.h	(revision 132380)
+++ avr.h	(working copy)
@@ -199,19 +199,29 @@
     1,1,/*  STACK */				\
     1,1 /* arg pointer */  }
 
-#define REG_ALLOC_ORDER {			\
-    24,25,					\
-    18,19,					\
-    20,21,					\
-    22,23,					\
-    30,31,					\
-    26,27,					\
-    28,29,					\
-    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,	\
-    0,1,					\
-    32,33,34,35					\
-    }
 
+#define REG_ALLOC_ORDER_0 {\
+    24,25,18,19,20,21,22,23,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }   
+  
+#define REG_ALLOC_ORDER_1 {\
+    18,19,20,21,22,23,24,25,30,31,26,27,28,29,\
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 }
+
+#define REG_ALLOC_ORDER_2 {\
+    18,22,20,24,19,23,21,25,30,31,26,27,28,29, \
+    17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,\
+    0,1,\
+    32,33,34,35 } 
+
+
+#define REG_ALLOC_ORDER REG_ALLOC_ORDER_0
+
+
 #define ORDER_REGS_FOR_LOCAL_ALLOC order_regs_for_local_alloc ()
 
 
@@ -453,11 +463,14 @@
 		           OPNUM, TYPE);				    \
 	      goto WIN;							    \
 	    }								    \
+        if(0)                                   \
+        {                                       \
 	  push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,	    \
 		       BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,	    \
 		       OPNUM, TYPE);					    \
           goto WIN;							    \
 	}								    \
+	}								    \
       else if (! (frame_pointer_needed && XEXP (X,0) == frame_pointer_rtx)) \
 	{								    \
 	  push_reload (X, NULL_RTX, &X, NULL,				    \
Index: avr.md
===================================================================
--- avr.md	(revision 132380)
+++ avr.md	(working copy)
@@ -251,8 +251,8 @@
    (set_attr "cc" "none")])
 
 (define_insn "*movhi"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,d,*r,q,r")
-        (match_operand:HI 1 "general_operand"       "r,m,rL,i,i,r,q"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,Qm,d,*r,q,r")
+        (match_operand:HI 1 "general_operand"       "r,Qm,rL,i,i,r,q"))]
   "(register_operand (operands[0],HImode)
     || register_operand (operands[1],HImode) || const0_rtx == operands[1])"
   "* return output_movhi (insn, operands, NULL);"

Re: Patch Fix PR35013, PR27192

[Andrew Hutchinson]

Anatoly ,

This patch was posted  to you for approval 2008-02-16

http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00644.html

Please can you comit this change.

best regards


Andrew Hutchinson wrote:
> The attached patch fixes
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35013
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27192
>
> Both of these problems occur because the avr backend fails to 
> recognize function pointer expressions - such as foo + 2
>
> As a result, these references are treated as byte addresses by 
> assembler, rather word addresses using the pm() directive. The patch 
> changes the operand check to one that finds the function reference 
> buried in the expression. (This is similar to code used on the SPARC-gcc)
>
> This patch has been tested using gcc version 4.3.0 20080121 
> (experimental)  with HEAD versions of the AVR files. No regressions 
> occurred running gcc.c-torture/execute testsuite with simulator and 
> the PR test cases generate the correct code.
>
> Please comit.
>
> 2008-02-16 Andy Hutchinson <hutchinsonandy@aim.com>
>
>    PR avr/35013 avr/27192
>
>    * avr.h (text_segment_operand): New function to check program 
> memory address.
>
>    * avr.c (text_segment_operand): New function to check program 
> memory address.
>    (print_operand_address,  avr_assemble_integer): Add new check.
>
>
>
>
>

Re:[PING^2] Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

Anatoly,

could you please approve this patch for AVR mainline so I can comit it?

Andy


Andrew Hutchinson wrote:
> I sent you this patch to approve and comit on 2008-02-2.
>
> It passed execute tests no regressions.
>
> Can it now be applied?
>
>
>
>
> Andrew Hutchinson wrote:
>> 2008-02-20  Andrew Hutchinson <hutchinsonandy@aim.com>
>>
>>    PR target/19636,24894,31644,31786
>>     * config/avr/avr.h (REG_ALLOC_ORDER_2):    Move defintion from
>>    avr.c. Re-order alternative 2.
>>    (LEGITIMIZE_RELOAD_ADDRESS): Don't push base pointer as reload for
>>    base pointer spill.
>>    * config/avr/avr.c (legitimate_address_p): Expose address inside
>>    subreg. Permit REG_X as base pointer.
>>    (order_regs_for_local_alloc) Move constant definitions to avr.h.
>>    (extra_constraint_Q): Add post-inc, pre-dec of X,Y Z to constraint.
>>    * config/avr/avr.md (*movhi) Add Q constraint before m.
>>
>>
>> I am re-testing modified patch. (It takes a few hours.)
>>
>>
>>

Re: [PING^2] Patch Fix PR35013, PR27192

[Andrew Hutchinson]

Please can you approve  so I can comit to avr mainline?

Andy


Andrew Hutchinson wrote:
> Anatoly ,
>
> This patch was posted  to you for approval 2008-02-16
>
> http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00644.html
>
> Please can you comit this change.
>
> best regards
>
>
> Andrew Hutchinson wrote:
>> The attached patch fixes
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35013
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27192
>>
>> Both of these problems occur because the avr backend fails to 
>> recognize function pointer expressions - such as foo + 2
>>
>> As a result, these references are treated as byte addresses by 
>> assembler, rather word addresses using the pm() directive. The patch 
>> changes the operand check to one that finds the function reference 
>> buried in the expression. (This is similar to code used on the 
>> SPARC-gcc)
>>
>> This patch has been tested using gcc version 4.3.0 20080121 
>> (experimental)  with HEAD versions of the AVR files. No regressions 
>> occurred running gcc.c-torture/execute testsuite with simulator and 
>> the PR test cases generate the correct code.
>>
>> Please comit.
>>
>> 2008-02-16 Andy Hutchinson <hutchinsonandy@aim.com>
>>
>>    PR avr/35013 avr/27192
>>
>>    * avr.h (text_segment_operand): New function to check program 
>> memory address.
>>
>>    * avr.c (text_segment_operand): New function to check program 
>> memory address.
>>    (print_operand_address,  avr_assemble_integer): Add new check.
>>
>>
>>
>>
>>
>

Re: Patch Fix PR35013, PR27192

[Andrew Hutchinson]

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

Here is predicate version - which is identical to SPARC and including  
gcc_unreachable() on switch.

I rebuild with this patch under Cygwin and Debian - with no issues. I 
ran c-torture/execute testsuite - no regressions.
and it still fixes the PR :-)

Please ignore wordwrap on changelog. Plain text format  in email is 
difficult to wrestle. I will make it fit 80 accordingly.

2008-04-06 Andy Hutchinson <hutchinsonandy@aim.com>

    PR avr/35013
    PR avr/27192
    * predicates.md (text_segment_operand): New function to check 
program memory address.
    * avr.c (avr_assemble_integer, print_operand_address): Use new 
predicate to check
program memory address.



Anatoly Sokolov wrote:
> Hi Andi.
>
>   
>> 2008-02-16 Andy Hutchinson <hutchinsonandy@aim.com>
>>
>>    PR avr/35013 avr/27192
>>     
>
> Shoud be:
>     PR avr/35013 
>     PR avr/27192
>
> In other ChageLogs also.
>
>   
>>    * avr.h (text_segment_operand): New function to check program memory 
>> address.
>>
>>    * avr.c (text_segment_operand): New function to check program memory 
>> address.
>>     
>
> Better define 'text_segment_operand' as predicate in predicates.md file.
>
>   
>>    (print_operand_address,  avr_assemble_integer): Add new check.
>>
>>
>>     
>
> I while study this patch.
>
> Anatoly.
>   

[-- Attachment #2: func-arith.patch --]
[-- Type: text/plain, Size: 1896 bytes --]

Index: gcc/config/avr/avr.c
===================================================================
--- gcc/config/avr/avr.c	(revision 133964)
+++ gcc/config/avr/avr.c	(working copy)
@@ -1123,8 +1123,7 @@
 
     default:
       if (CONSTANT_ADDRESS_P (addr)
-	  && ((GET_CODE (addr) == SYMBOL_REF && SYMBOL_REF_FUNCTION_P (addr))
-	      || GET_CODE (addr) == LABEL_REF))
+	  && text_segment_operand (addr, VOIDmode))
 	{
 	  fprintf (file, "gs(");
 	  output_addr_const (file,addr);
@@ -4477,8 +4476,7 @@
 avr_assemble_integer (rtx x, unsigned int size, int aligned_p)
 {
   if (size == POINTER_SIZE / BITS_PER_UNIT && aligned_p
-      && ((GET_CODE (x) == SYMBOL_REF && SYMBOL_REF_FUNCTION_P (x))
-	  || GET_CODE (x) == LABEL_REF))
+      && text_segment_operand (x, VOIDmode) )
     {
       fputs ("\t.word\tgs(", asm_out_file);
       output_addr_const (asm_out_file, x);
Index: gcc/config/avr/predicates.md
===================================================================
--- gcc/config/avr/predicates.md	(revision 133964)
+++ gcc/config/avr/predicates.md	(working copy)
@@ -71,6 +71,27 @@
 (define_predicate "symbol_ref_operand"
   (match_code "symbol_ref"))
 
+;; Return true if OP is a text segment reference.
+;; This is needed in the embedded medium/anywhere code model on V9.
+(define_predicate "text_segment_operand"
+  (match_code "label_ref,symbol_ref,plus,const")
+{
+  switch (GET_CODE (op))
+    {
+    case LABEL_REF :
+      return true;
+    case SYMBOL_REF :
+      return SYMBOL_REF_FUNCTION_P (op);
+    case PLUS :
+      /* Assume canonical format of symbol + constant.
+	 Fall through.  */
+    case CONST :
+      return text_segment_operand (XEXP (op, 0), VOIDmode);
+    default :
+      gcc_unreachable ();
+    }
+})
+
 ;; Return true if OP is a constant that contains only one 1 in its
 ;; binary representation.
 (define_predicate "single_one_operand"

Re: Re:[PING^2] Patch Fix PR1936,24894,31644,31786 AVR target

[Anatoly Sokolov]

Hi Andy,

please post me last version of this patch.

Anatoly.

----- Original Message ----- 
From: "Andrew Hutchinson" <andrewhutchinson@cox.net>
To: "Anatoly Sokolov" <aesok@post.ru>; <gcc-patches@gcc.gnu.org>
Cc: "Weddington, Eric" <eweddington@cso.atmel.com>
Sent: Sunday, April 06, 2008 7:35 PM
Subject: Re:[PING^2] Patch Fix PR1936,24894,31644,31786 AVR target


> Anatoly,
> 
> could you please approve this patch for AVR mainline so I can comit it?
> 
> Andy
> 
> 
> Andrew Hutchinson wrote:
>> I sent you this patch to approve and comit on 2008-02-2.
>>
>> It passed execute tests no regressions.
>>
>> Can it now be applied?
>>
>>
>>
>>
>> Andrew Hutchinson wrote:
>>> 2008-02-20  Andrew Hutchinson <hutchinsonandy@aim.com>
>>>
>>>    PR target/19636,24894,31644,31786
>>>     * config/avr/avr.h (REG_ALLOC_ORDER_2):    Move defintion from
>>>    avr.c. Re-order alternative 2.
>>>    (LEGITIMIZE_RELOAD_ADDRESS): Don't push base pointer as reload for
>>>    base pointer spill.
>>>    * config/avr/avr.c (legitimate_address_p): Expose address inside
>>>    subreg. Permit REG_X as base pointer.
>>>    (order_regs_for_local_alloc) Move constant definitions to avr.h.
>>>    (extra_constraint_Q): Add post-inc, pre-dec of X,Y Z to constraint.
>>>    * config/avr/avr.md (*movhi) Add Q constraint before m.
>>>
>>>
>>> I am re-testing modified patch. (It takes a few hours.)
>>>
>>>
>>>
>

Re: [PING^2] Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

That was last version as posted to gcc-patches

I have not changed it.

best regards

Andy




Anatoly Sokolov wrote:
> Hi Andy,
>
> please post me last version of this patch.
>
> Anatoly.
>
> ----- Original Message ----- 
> From: "Andrew Hutchinson" <andrewhutchinson@cox.net>
> To: "Anatoly Sokolov" <aesok@post.ru>; <gcc-patches@gcc.gnu.org>
> Cc: "Weddington, Eric" <eweddington@cso.atmel.com>
> Sent: Sunday, April 06, 2008 7:35 PM
> Subject: Re:[PING^2] Patch Fix PR1936,24894,31644,31786 AVR target
>
>
>   
>> Anatoly,
>>
>> could you please approve this patch for AVR mainline so I can comit it?
>>
>> Andy
>>
>>
>> Andrew Hutchinson wrote:
>>     
>>> I sent you this patch to approve and comit on 2008-02-2.
>>>
>>> It passed execute tests no regressions.
>>>
>>> Can it now be applied?
>>>
>>>
>>>
>>>
>>> Andrew Hutchinson wrote:
>>>       
>>>> 2008-02-20  Andrew Hutchinson <hutchinsonandy@aim.com>
>>>>
>>>>    PR target/19636,24894,31644,31786
>>>>     * config/avr/avr.h (REG_ALLOC_ORDER_2):    Move defintion from
>>>>    avr.c. Re-order alternative 2.
>>>>    (LEGITIMIZE_RELOAD_ADDRESS): Don't push base pointer as reload for
>>>>    base pointer spill.
>>>>    * config/avr/avr.c (legitimate_address_p): Expose address inside
>>>>    subreg. Permit REG_X as base pointer.
>>>>    (order_regs_for_local_alloc) Move constant definitions to avr.h.
>>>>    (extra_constraint_Q): Add post-inc, pre-dec of X,Y Z to constraint.
>>>>    * config/avr/avr.md (*movhi) Add Q constraint before m.
>>>>
>>>>
>>>> I am re-testing modified patch. (It takes a few hours.)
>>>>
>>>>
>>>>
>>>>         
> >

Re: [PING^3] Patch Fix PR1936,24894,31644,31786 AVR target

[Andrew Hutchinson]

Anatoly,

I'm  very patient but is still not been approved by you.
Original post was 2nd February.

Your last response to me on 24th May was that it was top of your list.

regards




Andrew Hutchinson wrote:
> That was last version as posted to gcc-patches
>
> I have not changed it.
>
> best regards
>
> Andy
>
>
>
>
> Anatoly Sokolov wrote:
>> Hi Andy,
>>
>> please post me last version of this patch.
>>
>> Anatoly.
>>
>> ----- Original Message ----- From: "Andrew Hutchinson" 
>> <andrewhutchinson@cox.net>
>> To: "Anatoly Sokolov" <aesok@post.ru>; <gcc-patches@gcc.gnu.org>
>> Cc: "Weddington, Eric" <eweddington@cso.atmel.com>
>> Sent: Sunday, April 06, 2008 7:35 PM
>> Subject: Re:[PING^2] Patch Fix PR1936,24894,31644,31786 AVR target
>>
>>
>>  
>>> Anatoly,
>>>
>>> could you please approve this patch for AVR mainline so I can comit it?
>>>
>>> Andy
>>>
>>>
>>> Andrew Hutchinson wrote:
>>>    
>>>> I sent you this patch to approve and comit on 2008-02-2.
>>>>
>>>> It passed execute tests no regressions.
>>>>
>>>> Can it now be applied?
>>>>
>>>>
>>>>
>>>>
>>>> Andrew Hutchinson wrote:
>>>>      
>>>>> 2008-02-20  Andrew Hutchinson <hutchinsonandy@aim.com>
>>>>>
>>>>>    PR target/19636,24894,31644,31786
>>>>>     * config/avr/avr.h (REG_ALLOC_ORDER_2):    Move defintion from
>>>>>    avr.c. Re-order alternative 2.
>>>>>    (LEGITIMIZE_RELOAD_ADDRESS): Don't push base pointer as reload for
>>>>>    base pointer spill.
>>>>>    * config/avr/avr.c (legitimate_address_p): Expose address inside
>>>>>    subreg. Permit REG_X as base pointer.
>>>>>    (order_regs_for_local_alloc) Move constant definitions to avr.h.
>>>>>    (extra_constraint_Q): Add post-inc, pre-dec of X,Y Z to 
>>>>> constraint.
>>>>>    * config/avr/avr.md (*movhi) Add Q constraint before m.
>>>>>
>>>>>
>>>>> I am re-testing modified patch. (It takes a few hours.)
>>>>>
>>>>>
>>>>>
>>>>>         
>> >
>