[patch,mips] avoid invalid register for JALR

[patch,mips] avoid invalid register for JALR

[Sandra Loosemore]

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

When I was trying to benchmark another patch (which I'll be sending 
along shortly) with CSiBE for -mabi=64, I ran into an assembler error 
like this:

/tmp/ccJv2faG.s: Assembler messages:
/tmp/ccJv2faG.s:1605: Error: a destination register must be supplied
`jalr $31'

Indeed, GCC is generating invalid code here; the single-operand JALR 
instruction doesn't permit the use of $31 because it is already the 
implicit destination register.  The attached patch introduces a new 
register class JALR_REGS to represent the valid set of registers for 
this instruction, and modifies the "c" register constraint to use it.

I had some difficulty in regression-testing this patch because of 
unrelated problems on trunk in the past week -- at first I was getting 
ICEs due to a null pointer dereference in tree code, then when I tried 
again a couple days later trunk was not even building.  So I ended up 
testing this patch on a more stable 4.9.0 checkout modified to support 
Mentor's extended set of mips-sde-elf multilibs instead.

OK to commit?

-Sandra


2014-05-13  Sandra Loosemore  <sandra@codesourcery.com>

	gcc/
	* config/mips/mips.h (enum reg_class): Add JALR_REGS.
	(REG_CLASS_NAMES): Add initializer for JALR_REGS.
	(REG_CLASS_CONTENTS): Likewise.
	* config/mips/constraints.md ("c"): Use JALR_REGS
	instead of GR_REGS.



[-- Attachment #2: mips-jalr.patch --]
[-- Type: text/x-patch, Size: 2006 bytes --]

Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	(revision 210372)
+++ gcc/config/mips/mips.h	(working copy)
@@ -1840,6 +1840,7 @@ enum reg_class
   PIC_FN_ADDR_REG,		/* SVR4 PIC function address register */
   V1_REG,			/* Register $v1 ($3) used for TLS access.  */
   LEA_REGS,			/* Every GPR except $25 */
+  JALR_REGS,			/* integer registers except $31 */	
   GR_REGS,			/* integer registers */
   FP_REGS,			/* floating point registers */
   MD0_REG,			/* first multiply/divide register */
@@ -1878,6 +1879,7 @@ enum reg_class
   "PIC_FN_ADDR_REG",							\
   "V1_REG",								\
   "LEA_REGS",								\
+  "JALR_REGS",								\
   "GR_REGS",								\
   "FP_REGS",								\
   "MD0_REG",								\
@@ -1919,6 +1921,7 @@ enum reg_class
   { 0x02000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 },	/* PIC_FN_ADDR_REG */	\
   { 0x00000008, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 },	/* V1_REG */		\
   { 0xfdffffff, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 },	/* LEA_REGS */		\
+  { 0x7fffffff, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 },	/* JALR_REGS */		\
   { 0xffffffff, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 },	/* GR_REGS */		\
   { 0x00000000, 0xffffffff, 0x00000000, 0x00000000, 0x00000000, 0x00000000 },	/* FP_REGS */		\
   { 0x00000000, 0x00000000, 0x00000001, 0x00000000, 0x00000000, 0x00000000 },	/* MD0_REG */		\
Index: gcc/config/mips/constraints.md
===================================================================
--- gcc/config/mips/constraints.md	(revision 210372)
+++ gcc/config/mips/constraints.md	(working copy)
@@ -50,7 +50,7 @@
 ;; for details.
 (define_register_constraint "c" "TARGET_MIPS16 ? M16_REGS
 				 : TARGET_USE_PIC_FN_ADDR_REG ? PIC_FN_ADDR_REG
-				 : GR_REGS"
+				 : JALR_REGS"
   "A register suitable for use in an indirect jump.  This will always be
    @code{$25} for @option{-mabicalls}.")
 

Re: [patch,mips] avoid invalid register for JALR

[Richard Sandiford]

Sandra Loosemore <sandra@codesourcery.com> writes:
> When I was trying to benchmark another patch (which I'll be sending 
> along shortly) with CSiBE for -mabi=64, I ran into an assembler error 
> like this:
>
> /tmp/ccJv2faG.s: Assembler messages:
> /tmp/ccJv2faG.s:1605: Error: a destination register must be supplied
> `jalr $31'

JALR patterns should have an explicit clobber of $31, which I thought
was also supposed to stop $31 from being used as the call address.  E.g.:

int foo (void)
{
  typedef void (*type) (void);
  register type fn asm ("$31");
  asm ("foo %0" : "=r" (fn));
  fn ();
  return 1;
}

gives the expected shuffle:

#APP
 # 5 "/tmp/foo.c" 1
	foo $31
 # 0 "" 2
#NO_APP
	move	$2,$31
	jalr	$2

If you change the asm to some other random register then the JALR
uses it directly.  Do you have a testcase?

> Indeed, GCC is generating invalid code here; the single-operand JALR 
> instruction doesn't permit the use of $31 because it is already the 
> implicit destination register.  The attached patch introduces a new 
> register class JALR_REGS to represent the valid set of registers for 
> this instruction, and modifies the "c" register constraint to use it.
>
> I had some difficulty in regression-testing this patch because of 
> unrelated problems on trunk in the past week -- at first I was getting 
> ICEs due to a null pointer dereference in tree code, then when I tried 
> again a couple days later trunk was not even building.

Could you give more details?

Thanks,
Richard

Re: [patch,mips] avoid invalid register for JALR

[Sandra Loosemore]

On 05/13/2014 03:41 PM, Richard Sandiford wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
>> When I was trying to benchmark another patch (which I'll be sending
>> along shortly) with CSiBE for -mabi=64, I ran into an assembler error
>> like this:
>>
>> /tmp/ccJv2faG.s: Assembler messages:
>> /tmp/ccJv2faG.s:1605: Error: a destination register must be supplied
>> `jalr $31'
>
> JALR patterns should have an explicit clobber of $31, which I thought
> was also supposed to stop $31 from being used as the call address.

Hmmmmm.  Yes, that ought to work, in theory....

> Do you have a testcase?

I can reproduce the error in a current mipsisa64-elfoabi build, with my 
patch to delete ADJUST_REG_ALLOC_ORDER applied.  It triggers on this 
file from CSiBE:

mipsisa64-elfoabi-gcc -c -mabi=64 -O2 -fno-common -w 
csibe/src/./ttt-0.10.1.preproc/src/connect4.i

>> I had some difficulty in regression-testing this patch because of
>> unrelated problems on trunk in the past week -- at first I was getting
>> ICEs due to a null pointer dereference in tree code, then when I tried
>> again a couple days later trunk was not even building.
>
> Could you give more details?

I think both of these problems have been fixed now, or at least I can 
build things again.  The tree null pointer dereference was probably the 
same thing you discussed here

https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00677.html

and the build failures seemed like this one

https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00500.html

-Sandra

Re: [patch,mips] avoid invalid register for JALR

[Maciej W. Rozycki]

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

On Wed, 14 May 2014, Sandra Loosemore wrote:

> > > When I was trying to benchmark another patch (which I'll be sending
> > > along shortly) with CSiBE for -mabi=64, I ran into an assembler error
> > > like this:
> > > 
> > > /tmp/ccJv2faG.s: Assembler messages:
> > > /tmp/ccJv2faG.s:1605: Error: a destination register must be supplied
> > > `jalr $31'
> > 
> > JALR patterns should have an explicit clobber of $31, which I thought
> > was also supposed to stop $31 from being used as the call address.
> 
> Hmmmmm.  Yes, that ought to work, in theory....
> 
> > Do you have a testcase?
> 
> I can reproduce the error in a current mipsisa64-elfoabi build, with my patch
> to delete ADJUST_REG_ALLOC_ORDER applied.  It triggers on this file from
> CSiBE:
> 
> mipsisa64-elfoabi-gcc -c -mabi=64 -O2 -fno-common -w
> csibe/src/./ttt-0.10.1.preproc/src/connect4.i

 I wonder if there's something fishy going on here.  I checked output 
produced with -dP and the offending instruction is emitted like this:

 #(call_insn 172 124 161 (parallel [
 #            (call (mem:SI (reg:DI 31 $31) [0 c4_setup S4 A32])
 #                (const_int 0 [0]))
 #            (clobber (reg:SI 31 $31))
 #        ]) c4_new.i:79 594 {call_internal}
 #     (expr_list:REG_DEAD (reg:DI 31 $31)
 #        (expr_list:REG_DEAD (reg:DI 7 $7)
 #            (expr_list:REG_DEAD (reg:DI 6 $6)
 #                (expr_list:REG_DEAD (reg:DI 5 $5)
 #                    (expr_list:REG_DEAD (reg:DI 4 $4)
 #                        (nil))))))
 #    (expr_list:DI (use (reg:DI 4 $4))
 #        (expr_list:SI (use (reg:DI 5 $5))
 #            (expr_list:SI (use (reg:DI 6 $6))
 #                (expr_list:SI (use (reg:DI 7 $7))
 #                    (nil))))))
	jalr	$31	 # 172	call_internal/1	[length = 4]

so clearly the clobber is ignored, or perhaps rather considered a late 
clobber instead of an early clobber that's indeed required here.

 I have reduced your case a bit and attached the result to this e-mail.  
With this code I can reproduce the problem using the following command:

$ mips-sde-elf-gcc -S -dP -mabi=64 -O2 -fno-common -w -o c4_new.s c4_new.i

and current trunk with the patch you recently posted as archived at:

https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01016.html

applied.  With the patch reverted the issue goes away ($17 is used for the 
jump), so clearly the register allocation order override made in 
mips_order_regs_for_local_alloc is currently covering an underlying bug.

  Maciej

[-- Attachment #2: Type: text/plain, Size: 3851 bytes --]

typedef unsigned int size_t;

extern void *malloc (size_t __size);

typedef char bool;
typedef struct _Board {
  struct _Board *(*copy) (const struct _Board *);  
  void (*setup) (struct _Board *, int, int, int);
  void (*display) (struct _Board *);  
  int (*eval) (struct _Board *);
  void (*score) (struct _Board *, int *, int *);  
  bool (*full) (struct _Board *);      
  int (*winner) (struct _Board *);     
  			 
  bool (*valid_move) (struct _Board *, int);  
  bool (*move) (struct _Board *, int, int);   
  void (*unmove) (struct _Board *);           
  int (*getmove) (struct _Board *, int, 
                int *, int *);  
  void (*help) (void);      
  char (*symbol) (struct _Board *, int, int);   
  void (*coords) (struct _Board *, int, char *);  
  int rows, cols;    
  int squares;	     
  int *moves;  
  int nummoves;  
  int X_player;      
  int *board;       
  int **points;	 
  int numpoints;
  int depth, depth2;   
  int *center;  
} Board;

Board *c4_copy (const Board *);
void c4_setup (Board *, int, int, int);
void c4_display (Board *);
int c4_eval (Board *);
inline bool c4_full (Board *);
int c4_winner (Board *);
bool c4_valid_move (Board *, int);
bool c4_move (Board *, int, int);
void c4_unmove (Board *);
int c4_getmove (Board *, int, int *, int *);
void c4_help (void);
char c4_symbol (Board *, int, int);
void c4_coords (Board *, int, char *);

Board *c4_new (int players, int size, int depth)
{
  Board *T = (Board *) malloc (sizeof (Board));
  Board *B = (Board *)T;
  int **points;
  int *board;
  int i, j, point = 0;
  int MyPE = 0;
  int rows, cols;

  B->copy = c4_copy;
  B->setup = c4_setup;
  B->display = c4_display;
  B->eval = c4_eval;
  B->full = c4_full;
  B->winner = c4_winner;
  B->valid_move = c4_valid_move;
  B->move = c4_move;
  B->unmove = c4_unmove;
  B->getmove = c4_getmove;
  B->help = c4_help;
  B->symbol = c4_symbol;
  B->coords = c4_coords;
  B->score = ((void *)0) ;

  rows = B->rows = 6;
  cols = B->cols = 7;
  B->squares = 7;  

  if (MyPE == 0)
    c4_setup ((Board *)T, players, depth, 0);

  B->board = (int *) malloc (rows*cols * sizeof(int));

  B->moves = (int *) malloc (rows*cols * sizeof(int));
  B->nummoves = 0;

  for (i = 0; i < rows*cols; i++)
    B->board[i] = B->moves[i] = 0;

  B->numpoints = rows*(cols-3) + cols*(rows-3) + 2*(rows-3)*(cols-3);
  points = (int **) malloc (B->numpoints * 4 * sizeof(int *));
  board = B->board;
   
  for (j = 0; j < rows - 3; j++) {
    for (i = 0; i < cols; i++, point++) {
      points[point*4+0] = board + i*rows + 0 + j;
      points[point*4+1] = board + i*rows + 1 + j;
      points[point*4+2] = board + i*rows + 2 + j;
      points[point*4+3] = board + i*rows + 3 + j;
    }
  }
  
  for (j = 0; j < cols - 3; j++) {
    for (i = 0; i < rows; i++, point++) {
      points[point*4+0] = board + i + 0*rows + j*rows;
      points[point*4+1] = board + i + 1*rows + j*rows;
      points[point*4+2] = board + i + 2*rows + j*rows;
      points[point*4+3] = board + i + 3*rows + j*rows;
    }
  }

  for (j = 0; j < cols - 3; j++) {
    for (i = 0; i < rows - 3; i++, point++) {
      points[point*4+0] = board + j*rows + i + 0*rows + 0;
      points[point*4+1] = board + j*rows + i + 1*rows + 1;
      points[point*4+2] = board + j*rows + i + 2*rows + 2;
      points[point*4+3] = board + j*rows + i + 3*rows + 3;
    }
  }
  for (j = 0; j < cols - 3; j++) {
    for (i = 0; i < rows - 3; i++, point++) {
      points[point*4+0] = board + j*rows + i + 0*rows + 3;
      points[point*4+1] = board + j*rows + i + 1*rows + 2;
      points[point*4+2] = board + j*rows + i + 2*rows + 1;
      points[point*4+3] = board + j*rows + i + 3*rows + 0;
    }
  }

  B->points = points;

  return T;
}