| Message ID | 53727BC3.3060600@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 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; }
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}.")
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.