This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFA:] Fix PR target/{18329,18330}, incorrect deletion of output-reload


The regression bug filed in PR target/18329 and PR target/18330
(actually the same bug, sorry for the duplicate) is a reload
bug, but fortunately not that hard to comprehend, I hope.

The MMIX port uses the initial-value machinery and explicit
restores from that initial-value pseudo for the return address
(register 259, rJ) after each call.  (To instead restore in the
return pattern or epilogue does not work, because the use of the
return address pseudo must be visible at time of register
allocation.)  The use of that pseudo gets a bit convoluted when
the execution path has a fork in a call because of a
nonlocal-goto that "returns" elsewhere than where the function
would return.  This doesn't seem to be the only way this bug can
be exposed, but since MMIX has this long-lived
return-address-pseudo and clobbering the return address is a bit
more prone to cause execution error than a temporary variable
with the wrong value, that may be the reason why this has not
shown up on other targets (or perhaps it has, but anyways the
bug is still there).

In 920501-7.c.22.lreg, we have this code, in short:
 save rJ (the return register) in pseudo 278.
 ... some insns not mentioning reg 278 or 259
 call y (a nested function)
 restore rJ from reg 278
 jump epilogue
nonlocal_label:
 nonlocal_goto_receiver_code
 restore rJ from reg 278
 jump epilogue

or if you prefer the more verbose RTL:

(insn 57 16 6 0 (set (reg:DI 278)
        (reg:DI 259 rJ)) 3 {movdi} (nil)
    (expr_list:REG_DEAD (reg:DI 259 rJ)
        (nil)))

... (other insns not mentioning reg 278 or 259)

(call_insn 25 24 54 0 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (parallel [
            (call (mem:QI (symbol_ref/v:DI ("@y::1042") <function_decl 0xf6f6815c y>) [0 S1 A8])
                (const_int 0 [0x0]))
            (use (reg 17 $17))
            (clobber (reg:DI 259 rJ))
        ]) 55 {*call_real} (insn_list:REG_DEP_TRUE 23 (insn_list:REG_DEP_TRUE 24 (nil)))
    (expr_list:REG_UNUSED (reg:DI 259 rJ)
        (expr_list:REG_DEAD (reg:DI 252 $252)
            (expr_list:REG_DEAD (reg:DI 16 $16)
                (expr_list:REG_UNUSED (reg:DI 259 rJ)
                    (nil)))))
    (expr_list:REG_DEP_TRUE (use (reg:DI 16 $16))
        (expr_list:REG_DEP_TRUE (use (reg:DI 252 $252))
            (nil))))
;; End of basic block 0, registers live:
 253 [$253] 254 [$254] 261 [ap_!BAD!] 269 278

;; Start of basic block 1, registers live: 253 [$253] 254 [$254] 261 [ap_!BAD!] 269 278
(note 54 25 26 1 [bb 1] NOTE_INSN_BASIC_BLOCK)

(insn 26 54 60 1 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (set (reg:DI 259 rJ)
        (reg:DI 278)) 3 {movdi} (nil)
    (expr_list:REG_DEAD (reg:DI 278)
        (nil)))

(jump_insn 60 26 61 1 (set (pc)
        (label_ref 38)) 59 {jump} (nil)
    (nil))
;; End of basic block 1, registers live:
 253 [$253] 254 [$254] 259 [rJ] 261 [ap_!BAD!] 269

(barrier 61 60 29)

;; Start of basic block 2, registers live: 253 [$253] 254 [$254] 261 [ap_!BAD!] 269 278
(code_label/s 29 61 36 2 3 "" [1 uses])

(note 36 29 30 2 [bb 2] NOTE_INSN_BASIC_BLOCK)

(insn 30 36 31 2 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (use (reg/f:DI 253 $253)) -1 (nil)
    (nil))

(insn 31 30 59 2 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (clobber (reg:DI 252 $252)) -1 (nil\
)
    (expr_list:REG_UNUSED (reg:DI 252 $252)
        (expr_list:REG_UNUSED (reg:DI 252 $252)
            (nil))))

(insn 59 31 33 2 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (set (reg/f:DI 253 $253)
        (plus:DI (reg/f:DI 253 $253)
            (const_int 24 [0x18]))) 7 {adddi3} (nil)
    (nil))

(insn 33 59 34 2 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (parallel [
            (unspec_volatile [
                    (const_int 0 [0x0])
                ] 1)
            (clobber (scratch:DI))
            (clobber (reg:DI 259 rJ))
        ]) 62 {*nonlocal_goto_receiver_expanded} (nil)
    (expr_list:REG_UNUSED (reg:DI 259 rJ)
        (expr_list:REG_UNUSED (scratch:DI)
            (expr_list:REG_UNUSED (reg:DI 259 rJ)
                (expr_list:REG_UNUSED (scratch:DI)
                    (nil))))))

(insn 34 33 35 2 /home/hp/combined/combined/gcc/testsuite/gcc.c-torture/execute/920501-7.c:17 (set (reg:DI 259 rJ)
        (reg:DI 278)) 3 {movdi} (nil)
    (expr_list:REG_DEAD (reg:DI 278)
        (nil)))

(The unspec_volatile is the main part of the MMIX nonlocal_goto_receiver
pattern.)

In global allocation, pseudo 278 gets allocated a stack slot, but as
memory is not valid for move to the return-address-register rJ, it is
reloaded into register 2, which happens to be valid and live unto the
death of pseudo 278 on the "straight" execution path.  When it's time to
emit reloads for insn 26 above (the rJ restore), GCC sees that reg 278
dies in that insn, so it gets clever and checks (by a call in
do_input_reloads) whether the first output reload into the stack-slot home
is actually necessary, or if register 2 would cover all uses.  It misses
the only such use, the nonlocal-goto case and so deletes the stack-slot
store of pseudo 278.  When the nonlocal-goto path restores rJ from that
stack-slot, it loads garbage (actually zero, in this case).

This correction is IMHO suitable for 4.0, being non-intrusive and in
effect disabling a faulty optimization.

Also changing the comment seemed appropriate, because as it stands, to me
it emphasizes on what paths the value can get *here* (the presumably-
last-use) rather than where the value can go from *there* (the presumably-
unused output reload).  If non-call-exceptions should be covered as well
(nonlocal-goto and calls should cover "normal C++" call-exceptions), that
should be done as a separate patch, IIUC similarly to this patch testing
whether in presence of nonlocal-goto labels and flag_non_call_exceptions,
any memory-accessing insns on the execution path are trapping, and if so
return.

Built and tested cross from i686-pc-linux-gnu (FC2) to
mmix-knuth-mmixware, no regressions.  Bootstrap and check native in
progress.

Ok to commit, assuming no regressions native?

	PR target/18329
	PR target/18330
	* reload1.c (delete_output_reload): Don't delete an output reload
	for a function with nonlocal-label and if there's a call on the
	execution path.  Adjust and improve wording of comment.


Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.458
diff -p -c -r1.458 reload1.c
*** reload1.c	6 Jan 2005 04:09:34 -0000	1.458
--- reload1.c	9 Jan 2005 12:19:59 -0000
*************** delete_output_reload (rtx insn, int j, i
*** 7624,7637 ****

    /* If the pseudo-reg we are reloading is no longer referenced
       anywhere between the store into it and here,
!      and no jumps or labels intervene, then the value can get
!      here through the reload reg alone.
       Otherwise, give up--return.  */
    for (i1 = NEXT_INSN (output_reload_insn);
         i1 != insn; i1 = NEXT_INSN (i1))
      {
        if (LABEL_P (i1) || JUMP_P (i1))
  	return;
        if ((NONJUMP_INSN_P (i1) || CALL_P (i1))
  	  && reg_mentioned_p (reg, PATTERN (i1)))
  	{
--- 7624,7646 ----

    /* If the pseudo-reg we are reloading is no longer referenced
       anywhere between the store into it and here,
!      and no jumps, calls or labels intervene, then the value can
!      only pass through the reload reg and end up here.
       Otherwise, give up--return.  */
    for (i1 = NEXT_INSN (output_reload_insn);
         i1 != insn; i1 = NEXT_INSN (i1))
      {
        if (LABEL_P (i1) || JUMP_P (i1))
  	return;
+       /* A call in a function where there are nonlocal labels, can
+ 	 return by a goto to such a label, and the pseudo might be
+ 	 used there, making it executionwise look like a branch.  When
+ 	 that happens, the original pseudo memory home is used there,
+ 	 not the reloaded register live at time of the call.
+ 	 ??? Can that happen for trapping memory accesses and
+ 	 non-call-exceptions as well?  */
+       if (CALL_P (i1) && current_function_has_nonlocal_label)
+ 	return;
        if ((NONJUMP_INSN_P (i1) || CALL_P (i1))
  	  && reg_mentioned_p (reg, PATTERN (i1)))
  	{

brgds, H-P


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]