When using -mflat -fno-delayed-branch and a function allocates stack space to save registers, the delay slot of the 'retl' return instruction is filled with the stack deallocation instruction. Because -fno-delayed-branch is specified, the stack deallocation should occur before the 'retl' and it's delay slot should get a 'nop'. Release: gcc-3.2.2 Environment: sparc-unknown-linux How-To-Repeat: Compile the testcase with -mflat -fno-delayed-branch on any 32-bit sparc target.
Fix: The -mflat epilogue output code needs to honor the delayed branch setting.
Hello, is this problem still present in gcc 3.3 and mainline? Thanks, Dara
See Dara's question.
I can confirm this on the mainline (20030714).
It is also in the mainline (20030823).
David could you test a patch for me as I do not have access to a sparc? Index: sparc.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/config/sparc/sparc.c,v retrieving revision 1.256 diff -u -p -r1.256 sparc.c --- sparc.c 16 Aug 2003 23:59:56 -0000 1.256 +++ sparc.c 24 Aug 2003 02:51:18 -0000 @@ -7223,11 +7223,13 @@ sparc_flat_function_epilogue (FILE *file fprintf (file, "\tset\t" HOST_WIDE_INT_PRINT_DEC ", %s\n", size, t1_str); } - - if (current_function_returns_struct) - fprintf (file, "\tjmp\t%%o7+12\n"); - else - fprintf (file, "\tretl\n"); + if (flag_delayed_branch) + { + if (current_function_returns_struct) + fprintf (file, "\tjmp\t%%o7+12\n"); + else + fprintf (file, "\tretl\n"); + } /* If the only register saved is the return address, we need a nop, unless we have an instruction to put into it. Otherwise @@ -7247,8 +7249,16 @@ sparc_flat_function_epilogue (FILE *file else if (size > 0) fprintf (file, "\tadd\t%s, %d, %s\n", sp_str, (int) size, sp_str); - else + else if (flag_delayed_branch) fprintf (file, "\tnop\n"); + + if (!flag_delayed_branch) + { + if (current_function_returns_struct) + fprintf (file, "\tjmp\t%%o7+12\n\tnop\n"); + else + fprintf (file, "\tretl\n\tnop\n"); + } } /* Reset state info for each function. */
It turns out that -fno-delayed-branch is not honored in the non-flat case either, so Andrew's patch is not sufficient.
I'm going to complete Andrew's patch and test it.
-fno-delayed-branch is not honored either by the following two peepholes: ;; Now peepholes to do a call followed by a jump. (define_peephole [(parallel [(set (match_operand 0 "" "") (call (mem:SI (match_operand:SI 1 "call_operand_address" "ps")) (match_operand 2 "" ""))) (clobber (reg:SI 15))]) (set (pc) (label_ref (match_operand 3 "" "")))] "short_branch (INSN_UID (insn), INSN_UID (operands[3])) && (USING_SJLJ_EXCEPTIONS || ! can_throw_internal (ins1)) && sparc_cpu != PROCESSOR_ULTRASPARC && sparc_cpu != PROCESSOR_ULTRASPARC3" "call\t%a1, %2\n\tadd\t%%o7, (%l3-.-4), %%o7") (define_peephole [(parallel [(call (mem:SI (match_operand:SI 0 "call_operand_address" "ps")) (match_operand 1 "" "")) (clobber (reg:SI 15))]) (set (pc) (label_ref (match_operand 2 "" "")))] "short_branch (INSN_UID (insn), INSN_UID (operands[2])) && (USING_SJLJ_EXCEPTIONS || ! can_throw_internal (ins1)) && sparc_cpu != PROCESSOR_ULTRASPARC && sparc_cpu != PROCESSOR_ULTRASPARC3" "call\t%a0, %1\n\tadd\t%%o7, (%l2-.-4), %%o7")
David, I have a patch but it doesn't fix all the cases (IIRC sibcalls and I'm not sure this is fixable). It also aims to prettify the assembly emitted for delay slots, i.e. always indent like in jmp %i7+8 restore while, currently, there is a mix of indentation and non-indentation. Would you mind if I re-target this bug for 3.5?
The sibcall thing is easy to fix, just turn sibcalls off in the sparc option handling fixup code if -fno-delayed-branch has been specified. No problem retargeting this to 3.5 I also agree with your delay slot indentation consolidation.
Agreed.
David, just to let you know that I've not forgotten this PR. I have a patch that almost works, except that there is a last problem: sparc_output_mi_thunk unconditionally uses a sibcall, which is forbidden with -fno-delayed-branches. Trying to find a workaround to this...
Created attachment 6695 [details] Tentative patch, not pretty though. David, here's the patch I could come up with. As you can see, it is not pretty because of the C++ thunk stuff. It passed a complete C,C++,ObjC testing cycle on sparc64-sun-solaris2.9 and sparc-sun-solaris2.8 with BOOT_CFLAGS="-g -O2 -fno-delayed-branch" CXXFLAGS="-g -O2 -fno-delayed-branch" so libstdc++-v3 was built without using delay slots (it aborted 3 times before I could get it to entirely compile). Do you have any other suggestion as to how the C++ thunk stuff must be dealt with? Do you think the patch is worth applying?
It may not be pretty, but I believe it solves the bug correctly. I think it is worth applying. If we come up with a more clever way to do this stuff, then we'll do so. Meanwhile the bug will be fixed, there will be no regressions, and the ugliness will be confined to the sparc backend.
Subject: Bug 10567 CVSROOT: /cvs/gcc Module name: gcc Changes by: ebotcazou@gcc.gnu.org 2004-07-07 21:33:09 Modified files: gcc : ChangeLog gcc/config/sparc: sparc.c sparc.md Log message: PR target/10567 * config/sparc/sparc.c (load_pic_register): Honor flag_delayed_branch. (output_return): Likewise. (output_sibcall): Abort if !flag_delayed_branch. (sparc_function_ok_for_sibcall): Return 0 if !flag_delayed_branch. (emit_and_preserve): New function. (sparc_output_mi_thunk): Use it. Honor flag_delayed_branch. Emit an indirect jump to the thunked-to function if !flag_delayed_branch. * config/sparc/sparc.md (delayed_branch): New attribute. (load_pcrel_sym): Honor flag_delayed_branch. Use above attribute to compute the length of the insn. (goto_handler_and_restore): Likewise. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.4349&r2=2.4350 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/sparc/sparc.c.diff?cvsroot=gcc&r1=1.311&r2=1.312 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/sparc/sparc.md.diff?cvsroot=gcc&r1=1.206&r2=1.207
So this is fixed and be closed, right?
I want to add a link to the gcc-patches message as usual. I sent it more than one hour ago but it seems to be blocked somewhere. Don't worry, I'll close the bug after the message has arrived.
Comment on attachment 6695 [details] Tentative patch, not pretty though. The patch uploaded in the audit trail is not quite correct, the C++ thunk support is broken.
See http://gcc.gnu.org/ml/gcc-patches/2004-07/msg00699.html
Subject: Bug 10567 CVSROOT: /cvs/gcc Module name: gcc Changes by: ebotcazou@gcc.gnu.org 2004-07-08 15:25:05 Modified files: gcc : ChangeLog gcc/config/sparc: sparc.md Log message: PR target/10567 * config/sparc/sparc.md (update_return): Honor flag_delayed_branch. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.4379&r2=2.4380 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/sparc/sparc.md.diff?cvsroot=gcc&r1=1.207&r2=1.208