Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Kenner]

    What we need to do is run some benchmarks and decide if LOAD_EXTEND_OP
    should be removed, or (much more likely) we remove (or obfuscate) the
    bits that claims that we can load SImode values into FP registers.

I'd very strongly recommend the latter.  I've seens lots of other problems
with SImode in FP.

Floating point registers vs. LOAD_EXTEND_OP on alpha

[Roger Sayle]

I believe I've managed to diagnose the cause of the current mainline
bootstrap failure on alphaev67-dec-osf5.1 and presumably all alpha
targets.

The failure was triggered by Joern's recent patch to postreload.c:

> 2004-01-16  J"orn Rennecke <joern.rennecke@superh.com>
>
>       PR 11864
>       * postreload.c (reload_cse_simplify_operands): Don't remove
>       implicit extension from LOAD_EXTEND_OP.

which causes a miscompilation of the garbage collection preprocessors,
such that gt-emit-rtl.h generated during stage2 contains garbage, such
that building emit-rtl.o in stage2 fails, terminating the bootstrap.

Running the regression tests from a non-bootstrapped compiler, and
comparing the results to a compiler with the appove patch reverted,
shows five new regressions, all gcc.c-torture/execute/memcpy-bi.c at
different optimization levels.

Comparing the assembly output of compiling memcpy-bi.c at -O3 shows
the following differences:

594a595
>       cvtlq $f2,$f2
716a718
>       cvtlq $f3,$f3
846a849
>       cvtlq $f4,$f4


The patch has eliminated the sign extensions of an integer value held
in the alpha's floating point registers.  Clearly there's some poor
interaction between LOAD_EXTEND_OP and loading integer values into FP
registers.

I'm guesing that in the sequence:

        lds $f2,40($10)
	cvtlq $f2,$f2
	sts $f2,40($9)

the "cvtlq" is actually doing something useful, such that the alpha
backend's definition of LOAD_EXTEND_OP depends upon register allocation?


Hopefully, this analysis is enough for an alpha maintainer to determine
whether Joern needs an additional check in his post-reload patch or if
the alpha backend's LOAD_EXTEND_OP should claim to be extending in these
modes.

Roger
--

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Henderson]

On Sun, Jan 18, 2004 at 10:18:59AM -0700, Roger Sayle wrote:
> 594a595
> >       cvtlq $f2,$f2
> 716a718
> >       cvtlq $f3,$f3
> 846a849
> >       cvtlq $f4,$f4
> 
> 
> The patch has eliminated the sign extensions of an integer value held
> in the alpha's floating point registers.

I hope you mean *added*.

> Clearly there's some poor interaction between LOAD_EXTEND_OP and
> loading integer values into FP registers.

Yes.  LOAD_EXTEND_OP is not true for the FP registers, only
the integer registers.  This is a problem...

> I'm guesing that in the sequence:
> 
>         lds $f2,40($10)
> 	cvtlq $f2,$f2
> 	sts $f2,40($9)
> 
> the "cvtlq" is actually doing something useful ...

No, the cvtlq insn there is actually corrupting data.

> Hopefully, this analysis is enough for an alpha maintainer to determine
> whether Joern needs an additional check in his post-reload patch or if
> the alpha backend's LOAD_EXTEND_OP should claim to be extending in these
> modes.

What we need to do is run some benchmarks and decide if LOAD_EXTEND_OP
should be removed, or (much more likely) we remove (or obfuscate) the
bits that claims that we can load SImode values into FP registers.


r~

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Henderson]

This patch is still in testing across 4 different alpha targets,
but it did at least 3-stage the compiler on alphaev67.  Just FYI.


r~


	* alpha.h (HARD_REGNO_MODE_OK): Disallow SImode in FP regs.
	* alpha.md (UNSPEC_NT_LDA): Remove.
	(UNSPEC_CVTLQ, cvtlq): New.
	(extendsidi2_1): Rename from extendsidi2_nofix; remove f/f.
	(extendsidi2_fix): Remove.
	(extendsidi2 splitter): Use cvtlq.
	(extendsidi2 fp peepholes): Remove.
	(cvtql): Use SFmode instead of SImode.
	(fix_trunc?fsi): Update to match.
	(floatsisf2_ieee, floatsisf2, floatsidf2_ieee, floatsidf2): New.
	(movsi): Rename from movsi_nofix, remove f alternatives.
	(movsi_nt_vms): Similarly.
	(movsi_fix, movsi_nt_vms_fix): Remove.
	(nt_lda): Remove.
	* alpha.c (alpha_expand_prologue): Use adddi3, not nt_lda.

Index: alpha.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/alpha/alpha.c,v
retrieving revision 1.342
diff -c -p -d -r1.342 alpha.c
*** alpha.c	13 Jan 2004 01:58:40 -0000	1.342
--- alpha.c	18 Jan 2004 22:17:25 -0000
*************** alpha_expand_prologue (void)
*** 7147,7161 ****
  	     and subtract it to sp. 
  
  	     Yes, that's correct -- we have to reload the whole constant
! 	     into a temporary via ldah+lda then subtract from sp.  To
! 	     ensure we get ldah+lda, we use a special pattern.  */
  
  	  HOST_WIDE_INT lo, hi;
  	  lo = ((frame_size & 0xffff) ^ 0x8000) - 0x8000;
  	  hi = frame_size - lo;
  
  	  emit_move_insn (ptr, GEN_INT (hi));
! 	  emit_insn (gen_nt_lda (ptr, GEN_INT (lo)));
  	  seq = emit_insn (gen_subdi3 (stack_pointer_rtx, stack_pointer_rtx,
  				       ptr));
  	}
--- 7147,7160 ----
  	     and subtract it to sp. 
  
  	     Yes, that's correct -- we have to reload the whole constant
! 	     into a temporary via ldah+lda then subtract from sp.  */
  
  	  HOST_WIDE_INT lo, hi;
  	  lo = ((frame_size & 0xffff) ^ 0x8000) - 0x8000;
  	  hi = frame_size - lo;
  
  	  emit_move_insn (ptr, GEN_INT (hi));
! 	  emit_insn (gen_adddi3 (ptr, ptr, GEN_INT (lo)));
  	  seq = emit_insn (gen_subdi3 (stack_pointer_rtx, stack_pointer_rtx,
  				       ptr));
  	}
Index: alpha.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/alpha/alpha.h,v
retrieving revision 1.213
diff -c -p -d -r1.213 alpha.h
*** alpha.h	15 Jan 2004 09:51:19 -0000	1.213
--- alpha.h	18 Jan 2004 22:17:25 -0000
*************** extern const char *alpha_tls_size_string
*** 614,625 ****
  
  /* Value is 1 if hard register REGNO can hold a value of machine-mode MODE.
     On Alpha, the integer registers can hold any mode.  The floating-point
!    registers can hold 32-bit and 64-bit integers as well, but not 16-bit
!    or 8-bit values.  */
  
  #define HARD_REGNO_MODE_OK(REGNO, MODE) 				\
    ((REGNO) >= 32 && (REGNO) <= 62 					\
!    ? GET_MODE_UNIT_SIZE (MODE) == 8 || GET_MODE_UNIT_SIZE (MODE) == 4	\
     : 1)
  
  /* Value is 1 if MODE is a supported vector mode.  */
--- 614,624 ----
  
  /* Value is 1 if hard register REGNO can hold a value of machine-mode MODE.
     On Alpha, the integer registers can hold any mode.  The floating-point
!    registers can hold 64-bit integers as well, but not smaller values.  */
  
  #define HARD_REGNO_MODE_OK(REGNO, MODE) 				\
    ((REGNO) >= 32 && (REGNO) <= 62 					\
!    ? (MODE) == SFmode || (MODE) == DFmode || (MODE) == DImode		\
     : 1)
  
  /* Value is 1 if MODE is a supported vector mode.  */
Index: alpha.md
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/alpha/alpha.md,v
retrieving revision 1.215
diff -c -p -d -r1.215 alpha.md
*** alpha.md	27 Oct 2003 06:49:41 -0000	1.215
--- alpha.md	18 Jan 2004 22:17:25 -0000
***************
*** 30,36 ****
     (UNSPEC_INSXH	2)
     (UNSPEC_MSKXH	3)
     (UNSPEC_CVTQL	4)
!    (UNSPEC_NT_LDA	5)
     (UNSPEC_UMK_LAUM	6)
     (UNSPEC_UMK_LALM	7)
     (UNSPEC_UMK_LAL	8)
--- 30,36 ----
     (UNSPEC_INSXH	2)
     (UNSPEC_MSKXH	3)
     (UNSPEC_CVTQL	4)
!    (UNSPEC_CVTLQ	5)
     (UNSPEC_UMK_LAUM	6)
     (UNSPEC_UMK_LALM	7)
     (UNSPEC_UMK_LAL	8)
***************
*** 185,225 ****
    ""
    "")
  
! (define_insn "*extendsidi2_nofix"
!   [(set (match_operand:DI 0 "register_operand" "=r,r,*f,?*f")
! 	(sign_extend:DI
! 	  (match_operand:SI 1 "nonimmediate_operand" "r,m,*f,m")))]
!   "! TARGET_FIX"
!   "@
!    addl $31,%1,%0
!    ldl %0,%1
!    cvtlq %1,%0
!    lds %0,%1\;cvtlq %0,%0"
!   [(set_attr "type" "iadd,ild,fadd,fld")
!    (set_attr "length" "*,*,*,8")])
  
! (define_insn "*extendsidi2_fix"
!   [(set (match_operand:DI 0 "register_operand" "=r,r,r,?*f,?*f")
  	(sign_extend:DI
! 	  (match_operand:SI 1 "nonimmediate_operand" "r,m,*f,*f,m")))]
!   "TARGET_FIX"
    "@
     addl $31,%1,%0
     ldl %0,%1
-    ftois %1,%0
-    cvtlq %1,%0
     lds %0,%1\;cvtlq %0,%0"
!   [(set_attr "type" "iadd,ild,ftoi,fadd,fld")
!    (set_attr "length" "*,*,*,*,8")])
  
- ;; Due to issues with CLASS_CANNOT_CHANGE_SIZE, we cannot use a subreg here.
  (define_split
    [(set (match_operand:DI 0 "hard_fp_register_operand" "")
  	(sign_extend:DI (match_operand:SI 1 "memory_operand" "")))]
    "reload_completed"
    [(set (match_dup 2) (match_dup 1))
!    (set (match_dup 0) (sign_extend:DI (match_dup 2)))]
!   "operands[2] = gen_rtx_REG (SImode, REGNO (operands[0]));")
  
  ;; Optimize sign-extension of SImode loads.  This shows up in the wake of
  ;; reload when converting fp->int.
--- 185,220 ----
    ""
    "")
  
! (define_insn "*cvtlq"
!   [(set (match_operand:DI 0 "register_operand" "=f")
! 	(unspec:DI [(match_operand:SF 1 "reg_or_0_operand" "fG")]
! 		   UNSPEC_CVTLQ))]
!   ""
!   "cvtlq %1,%0"
!   [(set_attr "type" "fadd")])
  
! (define_insn "*extendsidi2_1"
!   [(set (match_operand:DI 0 "register_operand" "=r,r,!*f")
  	(sign_extend:DI
! 	  (match_operand:SI 1 "nonimmediate_operand" "r,m,m")))]
!   ""
    "@
     addl $31,%1,%0
     ldl %0,%1
     lds %0,%1\;cvtlq %0,%0"
!   [(set_attr "type" "iadd,ild,fld")
!    (set_attr "length" "*,*,8")])
  
  (define_split
    [(set (match_operand:DI 0 "hard_fp_register_operand" "")
  	(sign_extend:DI (match_operand:SI 1 "memory_operand" "")))]
    "reload_completed"
    [(set (match_dup 2) (match_dup 1))
!    (set (match_dup 0) (unspec:DI [(match_dup 2)] UNSPEC_CVTLQ))]
! {
!   operands[1] = adjust_address (operands[1], SFmode, 0);
!   operands[2] = gen_rtx_REG (SFmode, REGNO (operands[0]));
! })
  
  ;; Optimize sign-extension of SImode loads.  This shows up in the wake of
  ;; reload when converting fp->int.
***************
*** 235,262 ****
  	(sign_extend:DI (match_dup 1)))]
    "")
  
- (define_peephole2
-   [(set (match_operand:SI 0 "hard_int_register_operand" "")
-         (match_operand:SI 1 "hard_fp_register_operand" ""))
-    (set (match_operand:DI 2 "hard_int_register_operand" "")
-         (sign_extend:DI (match_dup 0)))]
-   "TARGET_FIX
-    && (true_regnum (operands[0]) == true_regnum (operands[2])
-        || peep2_reg_dead_p (2, operands[0]))"
-   [(set (match_dup 2)
- 	(sign_extend:DI (match_dup 1)))]
-   "")
- 
- (define_peephole2
-   [(set (match_operand:DI 0 "hard_fp_register_operand" "")
-         (sign_extend:DI (match_operand:SI 1 "hard_fp_register_operand" "")))
-    (set (match_operand:DI 2 "hard_int_register_operand" "")
-         (match_dup 0))]
-   "TARGET_FIX && peep2_reg_dead_p (2, operands[0])"
-   [(set (match_dup 2)
- 	(sign_extend:DI (match_dup 1)))]
-   "")
- 
  ;; Don't say we have addsi3 if optimizing.  This generates better code.  We
  ;; have the anonymous addsi3 pattern below in case combine wants to make it.
  (define_expand "addsi3"
--- 230,235 ----
***************
*** 2334,2341 ****
  ;; processing, it is cheaper to do the truncation in the int regs.
  
  (define_insn "*cvtql"
!   [(set (match_operand:SI 0 "register_operand" "=f")
! 	(unspec:SI [(match_operand:DI 1 "reg_or_0_operand" "fG")]
  		   UNSPEC_CVTQL))]
    "TARGET_FP"
    "cvtql%/ %R1,%0"
--- 2307,2314 ----
  ;; processing, it is cheaper to do the truncation in the int regs.
  
  (define_insn "*cvtql"
!   [(set (match_operand:SF 0 "register_operand" "=f")
! 	(unspec:SF [(match_operand:DI 1 "reg_or_0_operand" "fG")]
  		   UNSPEC_CVTQL))]
    "TARGET_FP"
    "cvtql%/ %R1,%0"
***************
*** 2349,2362 ****
  	  (match_operator:DI 4 "fix_operator" 
  	    [(match_operand:DF 1 "reg_or_0_operand" "fG")]) 0))
     (clobber (match_scratch:DI 2 "=&f"))
!    (clobber (match_scratch:SI 3 "=&f"))]
    "TARGET_FP && alpha_fptm >= ALPHA_FPTM_SU"
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 4 [(match_dup 1)]))
!    (set (match_dup 3) (unspec:SI [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 0) (match_dup 3))]
!   ""
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
--- 2322,2337 ----
  	  (match_operator:DI 4 "fix_operator" 
  	    [(match_operand:DF 1 "reg_or_0_operand" "fG")]) 0))
     (clobber (match_scratch:DI 2 "=&f"))
!    (clobber (match_scratch:SF 3 "=&f"))]
    "TARGET_FP && alpha_fptm >= ALPHA_FPTM_SU"
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 4 [(match_dup 1)]))
!    (set (match_dup 3) (unspec:SF [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 5) (match_dup 3))]
! {
!   operands[5] = adjust_address (operands[0], SFmode, 0);
! }
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
***************
*** 2370,2379 ****
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 3 [(match_dup 1)]))
!    (set (match_dup 4) (unspec:SI [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 0) (match_dup 4))]
!   ;; Due to REG_CANNOT_CHANGE_SIZE issues, we cannot simply use SUBREG.
!   "operands[4] = gen_rtx_REG (SImode, REGNO (operands[2]));"
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
--- 2345,2356 ----
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 3 [(match_dup 1)]))
!    (set (match_dup 4) (unspec:SF [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 5) (match_dup 4))]
! {
!   operands[4] = gen_rtx_REG (SFmode, REGNO (operands[2]));
!   operands[5] = adjust_address (operands[0], SFmode, 0);
! }
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
***************
*** 2420,2433 ****
  	    [(float_extend:DF
  	       (match_operand:SF 1 "reg_or_0_operand" "fG"))]) 0))
     (clobber (match_scratch:DI 2 "=&f"))
!    (clobber (match_scratch:SI 3 "=&f"))]
    "TARGET_FP && alpha_fptm >= ALPHA_FPTM_SU"
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 4 [(float_extend:DF (match_dup 1))]))
!    (set (match_dup 3) (unspec:SI [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 0) (match_dup 3))]
!   ""
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
--- 2397,2412 ----
  	    [(float_extend:DF
  	       (match_operand:SF 1 "reg_or_0_operand" "fG"))]) 0))
     (clobber (match_scratch:DI 2 "=&f"))
!    (clobber (match_scratch:SF 3 "=&f"))]
    "TARGET_FP && alpha_fptm >= ALPHA_FPTM_SU"
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 4 [(float_extend:DF (match_dup 1))]))
!    (set (match_dup 3) (unspec:SF [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 4) (match_dup 3))]
! {
!   operands[4] = adjust_address (operands[0], SFmode, 0);
! }
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
***************
*** 2442,2451 ****
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 3 [(float_extend:DF (match_dup 1))]))
!    (set (match_dup 4) (unspec:SI [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 0) (match_dup 4))]
!   ;; Due to REG_CANNOT_CHANGE_SIZE issues, we cannot simply use SUBREG.
!   "operands[4] = gen_rtx_REG (SImode, REGNO (operands[2]));"
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
--- 2421,2432 ----
    "#"
    "&& reload_completed"
    [(set (match_dup 2) (match_op_dup 3 [(float_extend:DF (match_dup 1))]))
!    (set (match_dup 4) (unspec:SF [(match_dup 2)] UNSPEC_CVTQL))
!    (set (match_dup 5) (match_dup 4))]
! {
!   operands[4] = gen_rtx_REG (SFmode, REGNO (operands[2]));
!   operands[5] = adjust_address (operands[0], SFmode, 0);
! }
    [(set_attr "type" "fadd")
     (set_attr "trap" "yes")])
  
***************
*** 2516,2521 ****
--- 2497,2531 ----
     (set_attr "round_suffix" "normal")
     (set_attr "trap_suffix" "sui")])
  
+ (define_insn_and_split "*floatsisf2_ieee"
+   [(set (match_operand:SF 0 "register_operand" "=&f")
+ 	(float:SF (match_operand:SI 1 "memory_operand" "m")))
+    (clobber (match_scratch:DI 2 "=&f"))
+    (clobber (match_scratch:SF 3 "=&f"))]
+   "TARGET_FP && alpha_fptm >= ALPHA_FPTM_SU"
+   "#"
+   "&& reload_completed"
+   [(set (match_dup 3) (match_dup 1))
+    (set (match_dup 2) (unspec:DI [(match_dup 3)] UNSPEC_CVTLQ))
+    (set (match_dup 0) (float:SF (match_dup 2)))]
+ {
+   operands[1] = adjust_address (operands[1], SFmode, 0);
+ })
+ 
+ (define_insn_and_split "*floatsisf2"
+   [(set (match_operand:SF 0 "register_operand" "=f")
+ 	(float:SF (match_operand:SI 1 "memory_operand" "m")))]
+   "TARGET_FP"
+   "#"
+   "&& reload_completed"
+   [(set (match_dup 0) (match_dup 1))
+    (set (match_dup 2) (unspec:DI [(match_dup 0)] UNSPEC_CVTLQ))
+    (set (match_dup 0) (float:SF (match_dup 2)))]
+ {
+   operands[1] = adjust_address (operands[1], SFmode, 0);
+   operands[2] = gen_rtx_REG (DImode, REGNO (operands[0]));
+ })
+ 
  (define_insn "*floatdidf_ieee"
    [(set (match_operand:DF 0 "register_operand" "=&f")
  	(float:DF (match_operand:DI 1 "reg_no_subreg_operand" "f")))]
***************
*** 2536,2541 ****
--- 2546,2581 ----
     (set_attr "round_suffix" "normal")
     (set_attr "trap_suffix" "sui")])
  
+ (define_insn_and_split "*floatsidf2_ieee"
+   [(set (match_operand:DF 0 "register_operand" "=&f")
+ 	(float:DF (match_operand:SI 1 "memory_operand" "m")))
+    (clobber (match_scratch:DI 2 "=&f"))
+    (clobber (match_scratch:SF 3 "=&f"))]
+   "TARGET_FP && alpha_fptm >= ALPHA_FPTM_SU"
+   "#"
+   "&& reload_completed"
+   [(set (match_dup 3) (match_dup 1))
+    (set (match_dup 2) (unspec:DI [(match_dup 3)] UNSPEC_CVTLQ))
+    (set (match_dup 0) (float:DF (match_dup 2)))]
+ {
+   operands[1] = adjust_address (operands[1], SFmode, 0);
+ })
+ 
+ (define_insn_and_split "*floatsidf2"
+   [(set (match_operand:DF 0 "register_operand" "=f")
+ 	(float:DF (match_operand:SI 1 "memory_operand" "m")))]
+   "TARGET_FP"
+   "#"
+   "&& reload_completed"
+   [(set (match_dup 3) (match_dup 1))
+    (set (match_dup 2) (unspec:DI [(match_dup 3)] UNSPEC_CVTLQ))
+    (set (match_dup 0) (float:SF (match_dup 2)))]
+ {
+   operands[1] = adjust_address (operands[1], SFmode, 0);
+   operands[2] = gen_rtx_REG (DImode, REGNO (operands[0]));
+   operands[3] = gen_rtx_REG (SFmode, REGNO (operands[0]));
+ })
+ 
  (define_expand "floatditf2"
    [(use (match_operand:TF 0 "register_operand" ""))
     (use (match_operand:DI 1 "general_operand" ""))]
***************
*** 5166,5192 ****
      operands[1] = force_reg (TFmode, operands[1]);
  })
  
! (define_insn "*movsi_nofix"
!   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,m,*f,*f,m")
! 	(match_operand:SI 1 "input_operand" "rJ,K,L,m,rJ,*fJ,m,*f"))]
!   "(TARGET_ABI_OSF || TARGET_ABI_UNICOSMK) && ! TARGET_FIX
!    && (register_operand (operands[0], SImode)
!        || reg_or_0_operand (operands[1], SImode))"
!   "@
!    bis $31,%r1,%0
!    lda %0,%1($31)
!    ldah %0,%h1($31)
!    ldl %0,%1
!    stl %r1,%0
!    cpys %R1,%R1,%0
!    ld%, %0,%1
!    st%, %R1,%0"
!   [(set_attr "type" "ilog,iadd,iadd,ild,ist,fcpys,fld,fst")])
! 
! (define_insn "*movsi_fix"
!   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,m,*f,*f,m,r,*f")
! 	(match_operand:SI 1 "input_operand" "rJ,K,L,m,rJ,*fJ,m,*f,*f,r"))]
!   "TARGET_ABI_OSF && TARGET_FIX
     && (register_operand (operands[0], SImode)
         || reg_or_0_operand (operands[1], SImode))"
    "@
--- 5206,5215 ----
      operands[1] = force_reg (TFmode, operands[1]);
  })
  
! (define_insn "*movsi"
!   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,m")
! 	(match_operand:SI 1 "input_operand" "rJ,K,L,m,rJ"))]
!   "(TARGET_ABI_OSF || TARGET_ABI_UNICOSMK)
     && (register_operand (operands[0], SImode)
         || reg_or_0_operand (operands[1], SImode))"
    "@
***************
*** 5194,5231 ****
     lda %0,%1($31)
     ldah %0,%h1($31)
     ldl %0,%1
!    stl %r1,%0
!    cpys %R1,%R1,%0
!    ld%, %0,%1
!    st%, %R1,%0
!    ftois %1,%0
!    itofs %1,%0"
!   [(set_attr "type" "ilog,iadd,iadd,ild,ist,fcpys,fld,fst,ftoi,itof")])
! 
! (define_insn "*movsi_nt_vms_nofix"
!   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,r,m,*f,*f,m")
! 	(match_operand:SI 1 "input_operand" "rJ,K,L,s,m,rJ,*fJ,m,*f"))]
!   "(TARGET_ABI_WINDOWS_NT || TARGET_ABI_OPEN_VMS)
!     && !TARGET_FIX
!     && (register_operand (operands[0], SImode)
!         || reg_or_0_operand (operands[1], SImode))"
!   "@
!    bis $31,%1,%0
!    lda %0,%1
!    ldah %0,%h1
!    lda %0,%1
!    ldl %0,%1
!    stl %r1,%0
!    cpys %R1,%R1,%0
!    ld%, %0,%1
!    st%, %R1,%0"
!   [(set_attr "type" "ilog,iadd,iadd,ldsym,ild,ist,fcpys,fld,fst")])
  
! (define_insn "*movsi_nt_vms_fix"
!   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,r,m,*f,*f,m,r,*f")
! 	(match_operand:SI 1 "input_operand" "rJ,K,L,s,m,rJ,*fJ,m,*f,*f,r"))]
    "(TARGET_ABI_WINDOWS_NT || TARGET_ABI_OPEN_VMS)
-     && TARGET_FIX
      && (register_operand (operands[0], SImode)
          || reg_or_0_operand (operands[1], SImode))"
    "@
--- 5217,5229 ----
     lda %0,%1($31)
     ldah %0,%h1($31)
     ldl %0,%1
!    stl %r1,%0"
!   [(set_attr "type" "ilog,iadd,iadd,ild,ist")])
  
! (define_insn "*movsi_nt_vms"
!   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,r,m")
! 	(match_operand:SI 1 "input_operand" "rJ,K,L,s,m,rJ"))]
    "(TARGET_ABI_WINDOWS_NT || TARGET_ABI_OPEN_VMS)
      && (register_operand (operands[0], SImode)
          || reg_or_0_operand (operands[1], SImode))"
    "@
***************
*** 5234,5246 ****
     ldah %0,%h1
     lda %0,%1
     ldl %0,%1
!    stl %r1,%0
!    cpys %R1,%R1,%0
!    ld%, %0,%1
!    st%, %R1,%0
!    ftois %1,%0
!    itofs %1,%0"
!   [(set_attr "type" "ilog,iadd,iadd,ldsym,ild,ist,fcpys,fld,fst,ftoi,itof")])
  
  (define_insn "*movhi_nobwx"
    [(set (match_operand:HI 0 "register_operand" "=r,r")
--- 5232,5239 ----
     ldah %0,%h1
     lda %0,%1
     ldl %0,%1
!    stl %r1,%0"
!   [(set_attr "type" "ilog,iadd,iadd,ldsym,ild,ist")])
  
  (define_insn "*movhi_nobwx"
    [(set (match_operand:HI 0 "register_operand" "=r,r")
***************
*** 6900,6916 ****
    alpha_expand_epilogue ();
    DONE;
  })
- 
- ;; In creating a large stack frame, NT _must_ use ldah+lda to load
- ;; the frame size into a register.  We use this pattern to ensure
- ;; we get lda instead of addq.
- (define_insn "nt_lda"
-   [(set (match_operand:DI 0 "register_operand" "=r")
- 	(unspec:DI [(match_dup 0)
- 		    (match_operand:DI 1 "const_int_operand" "n")]
- 		   UNSPEC_NT_LDA))]
-   ""
-   "lda %0,%1(%0)")
  
  (define_expand "builtin_longjmp"
    [(use (match_operand:DI 0 "register_operand" "r"))]
--- 6893,6898 ----

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Joern Rennecke]

> Hopefully, this analysis is enough for an alpha maintainer to determine
> whether Joern needs an additional check in his post-reload patch or if
> the alpha backend's LOAD_EXTEND_OP should claim to be extending in these
> modes.

I could check
CANNOT_CHANGE_MODE_CLASS (GET_MODE (op), word_mode,
			  REGNO_REG_CLASS (REGNO (SET_DEST (set)))) ,
but there is similar code in reload_cse_simplify_set that would also
need to be changed, and we'd have to document that LOAD_EXTEND_OP
does not apply to hard registers whose reg class cannot change to
word_mode or any integral integer mode between the mode of the
memory reference and word_mode. (If it could do the latter change,
we'd need data flow analysis to find out if an extension is suposed
to take place).

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Henderson]

On Mon, Jan 19, 2004 at 08:32:52PM +0000, Joern Rennecke wrote:
> I could check
> CANNOT_CHANGE_MODE_CLASS (GET_MODE (op), word_mode,
> 			  REGNO_REG_CLASS (REGNO (SET_DEST (set)))) ,
> but there is similar code in reload_cse_simplify_set that would also
> need to be changed, and we'd have to document that LOAD_EXTEND_OP
> does not apply to hard registers whose reg class cannot change to
> word_mode or any integral integer mode between the mode of the
> memory reference and word_mode.

Don't bother.  I've just fixed the backend to not accept SImode
values in FP registers at all.  As Kenner said upthread, we've
seen problems with this in the past; your change was just the
straw that broke the camel's back.


r~

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Sandiford]

Richard Henderson <rth@redhat.com> writes:
> On Mon, Jan 19, 2004 at 08:32:52PM +0000, Joern Rennecke wrote:
>> I could check
>> CANNOT_CHANGE_MODE_CLASS (GET_MODE (op), word_mode,
>> 			  REGNO_REG_CLASS (REGNO (SET_DEST (set)))) ,
>> but there is similar code in reload_cse_simplify_set that would also
>> need to be changed, and we'd have to document that LOAD_EXTEND_OP
>> does not apply to hard registers whose reg class cannot change to
>> word_mode or any integral integer mode between the mode of the
>> memory reference and word_mode.
>
> Don't bother.  I've just fixed the backend to not accept SImode
> values in FP registers at all.  As Kenner said upthread, we've
> seen problems with this in the past; your change was just the
> straw that broke the camel's back.

Well, the same problem applies to MIPS as well.  We specifically
include FP_REGS in C_C_M_C for this very reason.

Or should MIPS be changed in the same way as alpha?  (Pretty ironic if so,
since we used for forbid SImode values in FPRs, but I changed that under
advice that this shouldn't be a problem. ;)

On the other hand, what's the point of having CANNOT_CHANGE_MODE_CLASS
if post-reload passes are allowed to change the mode of a register like
this?  Unextended registers aren't the only thing that could go wrong on
MIPS if you ignore the macro.  E.g. the HI/LO registers are always
ordered that way round, even on little-endian systems.  I know it
should be changed, but ISTR other ports have similar warts...

FWIW, before Joern's change, I haven't seen any problems with FPRs
being incorrectly treated as sign-extended.  I haven't yet tested
after the patch.

Richard

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Henderson]

On Tue, Jan 20, 2004 at 09:30:53AM +0000, Richard Sandiford wrote:
> Or should MIPS be changed in the same way as alpha?  (Pretty ironic if so,
> since we used for forbid SImode values in FPRs, but I changed that under
> advice that this shouldn't be a problem. ;)

:-)

> On the other hand, what's the point of having CANNOT_CHANGE_MODE_CLASS
> if post-reload passes are allowed to change the mode of a register like
> this?  Unextended registers aren't the only thing that could go wrong on
> MIPS if you ignore the macro.  E.g. the HI/LO registers are always
> ordered that way round, even on little-endian systems.  I know it
> should be changed, but ISTR other ports have similar warts...

You make a good case.  Also, the fact that it's more than just
Alpha and PA makes me wonder what other port will have issues.

Joern, why don't you go ahead and work something up?


r~

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Joern Rennecke]

> > On the other hand, what's the point of having CANNOT_CHANGE_MODE_CLASS
> > if post-reload passes are allowed to change the mode of a register like
> > this?  Unextended registers aren't the only thing that could go wrong on
> > MIPS if you ignore the macro.  E.g. the HI/LO registers are always
> > ordered that way round, even on little-endian systems.  I know it
> > should be changed, but ISTR other ports have similar warts...
> 
> You make a good case.  Also, the fact that it's more than just
> Alpha and PA makes me wonder what other port will have issues.
> 
> Joern, why don't you go ahead and work something up?

I've found that the other hunk in postreload was already guarded with a
test of CLASS_CANNOT_CHANGE_MODE, but without reflecting the potential
or dangers of this check in the documentation.  N.B. without the
documentation change, it is allowed that a port says some QImode
register can be extended to HImode, but can't be extended to SImode,
which is word_mode, while makeing LOAD_EXTEND_OP SIGN_EXTEND.
Then combine might make use of an implicit QI->HI sign extension, while
psotreload decides that the extension doesn't exist, because it doesn't
extend to word_mode.

I don't have any access to mips or alpha machines, so could someone else
test this?

2004-01-20  J"orn Rennecke <joern.rennecke@superh.com>

	* doc/tm.texi: Insert some weasel words when LOAD_EXTEND_OP
	may or may not return non-NIL.
	* postreload.c ():

Index: doc/tm.texi
===================================================================
RCS file: /cvs/gcc/gcc/gcc/doc/tm.texi,v
retrieving revision 1.284
diff -p -r1.284 tm.texi
*** doc/tm.texi	18 Jan 2004 11:57:16 -0000	1.284
--- doc/tm.texi	20 Jan 2004 18:52:05 -0000
*************** smaller than a word are always performed
*** 8498,8517 ****
  Most RISC machines have this property and most CISC machines do not.
  @end defmac
  
! @defmac LOAD_EXTEND_OP (@var{mode})
  Define this macro to be a C expression indicating when insns that read
! memory in @var{mode}, an integral mode narrower than a word, set the
! bits outside of @var{mode} to be either the sign-extension or the
  zero-extension of the data read.  Return @code{SIGN_EXTEND} for values
! of @var{mode} for which the
  insn sign-extends, @code{ZERO_EXTEND} for which it zero-extends, and
  @code{NIL} for other modes.
  
! This macro is not called with @var{mode} non-integral or with a width
  greater than or equal to @code{BITS_PER_WORD}, so you may return any
  value in this case.  Do not define this macro if it would always return
  @code{NIL}.  On machines where this macro is defined, you will normally
  define it as the constant @code{SIGN_EXTEND} or @code{ZERO_EXTEND}.
  @end defmac
  
  @defmac SHORT_IMMEDIATES_SIGN_EXTEND
--- 8498,8528 ----
  Most RISC machines have this property and most CISC machines do not.
  @end defmac
  
! @defmac LOAD_EXTEND_OP (@var{mem_mode})
  Define this macro to be a C expression indicating when insns that read
! memory in @var{mem_mode}, an integral mode narrower than a word, set the
! bits outside of @var{mem_mode} to be either the sign-extension or the
  zero-extension of the data read.  Return @code{SIGN_EXTEND} for values
! of @var{mem_mode} for which the
  insn sign-extends, @code{ZERO_EXTEND} for which it zero-extends, and
  @code{NIL} for other modes.
  
! This macro is not called with @var{mem_mode} non-integral or with a width
  greater than or equal to @code{BITS_PER_WORD}, so you may return any
  value in this case.  Do not define this macro if it would always return
  @code{NIL}.  On machines where this macro is defined, you will normally
  define it as the constant @code{SIGN_EXTEND} or @code{ZERO_EXTEND}.
+ 
+ You may return a non-@code{NIL} value even if for some hard registers
+ the sign extension is not performed, if for the @code{REGNO_REG_CLASS}
+ of these hard registers @code{CANNOT_CHANGE_MODE_CLASS} returns nonzero
+ when the @var{from} mode is @var{mem_mode} and the @var{to} mode is any
+ integral mode larger than this but not larger than @code{word_mode}.
+ 
+ You must return @code{NIL} if form some hard registers that allow this
+ mode, @code{CANNOT_CHANGE_MODE_CLASS} says that they cannot change to
+ @code{word_mode}, but that they can change to another integral mode that
+ is larger then @var{mem_mode} but still smaller than @code{word_mode}.
  @end defmac
  
  @defmac SHORT_IMMEDIATES_SIGN_EXTEND
Index: postreload.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/postreload.c,v
retrieving revision 2.9
diff -p -r2.9 postreload.c
*** postreload.c	16 Jan 2004 16:11:56 -0000	2.9
--- postreload.c	20 Jan 2004 18:52:05 -0000
*************** reload_cse_simplify_operands (rtx insn, 
*** 423,428 ****
--- 423,437 ----
  		   || GET_CODE (SET_SRC (set)) == ZERO_EXTEND
  		   || GET_CODE (SET_SRC (set)) == SIGN_EXTEND)
  	    ; /* Continue ordinary processing.  */
+ #ifdef CANNOT_CHANGE_MODE_CLASS
+ 	  /* If the register cannot change mode to word_mode, it follows that
+ 	     it cannot have been used in word_mode.  */
+ 	  else if (GET_CODE (SET_DEST (set)) == REG
+ 		   && CANNOT_CHANGE_MODE_CLASS (GET_MODE (SET_DEST (set)),
+ 						word_mode,
+ 						REGNO_REG_CLASS (REGNO (SET_DEST (set)))))
+ 	    ; /* Continue ordinary processing.  */
+ #endif
  	  /* If this is a straight load, make the extension explicit.  */
  	  else if (GET_CODE (SET_DEST (set)) == REG
  		   && recog_data.n_operands == 2

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Joern Rennecke]

> 	* postreload.c ():

Oops, that was a bit terse.

	* postreload.c (reload_cse_simplify_operands): In LOAD_EXTEND_OP code,
	check CANNOT_CHANGE_MODE_CLASS

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Eric Botcazou]

> 	* postreload.c (reload_cse_simplify_operands): In LOAD_EXTEND_OP code,
> 	check CANNOT_CHANGE_MODE_CLASS

Why did you not put it on the 3.4 branch?

-- 
Eric Botcazou

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Joern Rennecke]

> > 	* postreload.c (reload_cse_simplify_operands): In LOAD_EXTEND_OP code,
> > 	check CANNOT_CHANGE_MODE_CLASS
> 
> Why did you not put it on the 3.4 branch?

Isn't it up to the release manager to specify which patches he wants to
be applied to the branch?

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Gerald Pfeifer]

On Mon, 26 Jan 2004, Joern Rennecke wrote:
> Isn't it up to the release manager to specify which patches he wants to
> be applied to the branch?

Currently the 3.4 branch is open for regression fixes.

(In general I try to keep the current status of all active release
branches on the GCC main web page, so that's a good reference. ;-) )

Gerald

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Joern Rennecke]

> 
> On Mon, 26 Jan 2004, Joern Rennecke wrote:
> > Isn't it up to the release manager to specify which patches he wants to
> > be applied to the branch?
> 
> Currently the 3.4 branch is open for regression fixes.

Ok, I have checked the patch in, relying on Richard Sandiford's
regression test.

The 3.3 branch is also marked as open for bug fixes, but
we still need somebody to test the two patches in the 3.3 branch
on one of the affected systems to get closure on PR optimization/11864.

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Eric Botcazou]

> Ok, I have checked the patch in, relying on Richard Sandiford's
> regression test.

Thanks.

> The 3.3 branch is also marked as open for bug fixes, but
> we still need somebody to test the two patches in the 3.3 branch
> on one of the affected systems to get closure on PR optimization/11864.

FYI, the patches will probably require a change to the SPARC back-end, 
because it is affected by the very same problem as the MIPS and PA 
back-ends, but currently doesn't define CANNOT_CHANGE_MODE_CLASS.
So I don't think the patches are suitable for the 3.3 branch.

-- 
Eric Botcazou

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Gabriel Dos Reis]

Eric Botcazou <ebotcazou@libertysurf.fr> writes:

| > Ok, I have checked the patch in, relying on Richard Sandiford's
| > regression test.
| 
| Thanks.
| 
| > The 3.3 branch is also marked as open for bug fixes, but
| > we still need somebody to test the two patches in the 3.3 branch
| > on one of the affected systems to get closure on PR optimization/11864.
| 
| FYI, the patches will probably require a change to the SPARC back-end, 
| because it is affected by the very same problem as the MIPS and PA 
| back-ends, but currently doesn't define CANNOT_CHANGE_MODE_CLASS.
| So I don't think the patches are suitable for the 3.3 branch.

This patch, if it were to be backported to the 3.3.x series, would not
happen before 3.3.3 is out.  I've already missed the release date y a
week.

Thanks,

-- Gaby

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Eric Botcazou]

> Isn't it up to the release manager to specify which patches he wants to
> be applied to the branch?

Sure, but leaving a patch which is known to cause problems on a branch is not 
very nice.  I think you should either put the complete fix or revert the 
first half.

-- 
Eric Botcazou

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Sandiford]

Joern Rennecke <joern.rennecke@superh.com> writes:
> I don't have any access to mips or alpha machines, so could someone else
> test this?

Bootstrapped & regression tested on mips64-linux-gnu (3.4 branch)
without problems.  Thanks for the patch.

Richard

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Henderson]

On Thu, Jan 22, 2004 at 09:39:56AM +0000, Richard Sandiford wrote:
> Joern Rennecke <joern.rennecke@superh.com> writes:
> > I don't have any access to mips or alpha machines, so could someone else
> > test this?
> 
> Bootstrapped & regression tested on mips64-linux-gnu (3.4 branch)
> without problems.  Thanks for the patch.

Cool.  I think I'm not going to revert the Alpha changes regardless,
so this oughtn't make a difference there.

Joern, go ahead and commit the patch.


r~

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Joern Rennecke]

> Hopefully, this analysis is enough for an alpha maintainer to determine
> whether Joern needs an additional check in his post-reload patch or if
> the alpha backend's LOAD_EXTEND_OP should claim to be extending in these
> modes.

P.S.: A more useful change of LOAD_EXTEND_OP would say that the extension
only widens as far as it can without increasing the number of hard registers.
Or that it just doesn't apply to registers that are smaller than word_mode.

That would allow the SHmedia port to use LOAD_EXTEND_OP for SImode.

That leaves the question how to best implement this in the register
allocator.

Maybe we are just trying to hard to keep a nice hack working without changing
the way the RTL looks during combine.  It would seem that things would
be much saner if we required that an extend pattern much exist to match
LOAD_EXTEND_OP for it to be effective, and when combine makes use if it, it
replaces the load with a SIGN_EXTEND / ZERO_EXTEND as appropriate.
(The code in cse does appear to already replace some MEMs with a
 copy from a sign / zero extension, rather than the other way round.)

The main problem with this approach seems to keep track of when an
extension is actually used.  The tangle of last_value and cached
sign and 'non'zero bits makes me wonder if it might be easier to make
a register width aware flow pass for arbitrary integer modes <= word_mode.

Re: Floating point registers vs. LOAD_EXTEND_OP on alpha

[Richard Henderson]

On Mon, Jan 19, 2004 at 09:24:59PM +0000, Joern Rennecke wrote:
> Maybe we are just trying to hard to keep a nice hack working without changing
> the way the RTL looks during combine.  It would seem that things would
> be much saner if we required that an extend pattern much exist to match
> LOAD_EXTEND_OP for it to be effective, and when combine makes use if it, it
> replaces the load with a SIGN_EXTEND / ZERO_EXTEND as appropriate.
> (The code in cse does appear to already replace some MEMs with a
>  copy from a sign / zero extension, rather than the other way round.)

Indeed, my preference would be to move away from on-the-side
macros like LOAD_EXTEND_OP as much as possible, in favour of
direct representation in the IL.

> ... makes me wonder if it might be easier to make
> a register width aware flow pass for arbitrary integer modes <= word_mode.

The thought has ocurred to me in the past.


r~