Re: [Patch] Fix bug for frame instructions in annulled delay slots
On 12/07/2015 02:18 PM, Steve Ellcey wrote: On Mon, 2015-12-07 at 12:30 -0700, Jeff Law wrote: On 12/07/2015 12:28 PM, Bernd Schmidt wrote: On 12/07/2015 07:54 PM, Steve Ellcey wrote: if (must_annul) -used_annul = 1; +{ + /* Frame related instructions cannot go into annulled delay + slots, it messes up the dwarf info. */ + if (RTX_FRAME_RELATED_P (trial)) +return; Don't you need to use break rather than return? + else if (!RTX_FRAME_RELATED_P (trial) \ Stray backslash Other than that I think this is OK. There are some preexisting tests for frame related insns already in this code. Also note there's probably port cleanup that could happen once this goes in. IIRC the PA port (for example) explicitly disallows frame related insns from many (most, all?) delay slots. Other targets may be doing something similar. jeff If I had remembered/seen that code in pa.md earlier I might have fixed MIPS the same way. Oh well, I guess changing reorg.c is the better way to do it since it can be shared now. Agreed. executed when the branch is taken (vs. untaken). I think steal_delay_list_from_fallthrough is only going to be used when the delay slot is executed on a branch-not-taken. Are there any machines like that? ? I don't think it's restricted to cases where the delay slot is executed only on a branch-not-taken. Jeff
Re: [Patch] Fix bug for frame instructions in annulled delay slots
On 12/07/2015 07:54 PM, Steve Ellcey wrote: if (must_annul) - used_annul = 1; + { + /* Frame related instructions cannot go into annulled delay +slots, it messes up the dwarf info. */ + if (RTX_FRAME_RELATED_P (trial)) + return; Don't you need to use break rather than return? + else if (!RTX_FRAME_RELATED_P (trial) \ Stray backslash. Other than that I think this is OK. There are some preexisting tests for frame related insns already in this code. Bernd
Re: [Patch] Fix bug for frame instructions in annulled delay slots
On 12/07/2015 12:28 PM, Bernd Schmidt wrote: On 12/07/2015 07:54 PM, Steve Ellcey wrote: if (must_annul) -used_annul = 1; +{ + /* Frame related instructions cannot go into annulled delay + slots, it messes up the dwarf info. */ + if (RTX_FRAME_RELATED_P (trial)) +return; Don't you need to use break rather than return? + else if (!RTX_FRAME_RELATED_P (trial) \ Stray backslash. Other than that I think this is OK. There are some preexisting tests for frame related insns already in this code. Also note there's probably port cleanup that could happen once this goes in. IIRC the PA port (for example) explicitly disallows frame related insns from many (most, all?) delay slots. Other targets may be doing something similar. jeff
Re: [Patch] Fix bug for frame instructions in annulled delay slots
On Mon, 2015-12-07 at 20:28 +0100, Bernd Schmidt wrote: > On 12/07/2015 07:54 PM, Steve Ellcey wrote: > > if (must_annul) > > - used_annul = 1; > > + { > > + /* Frame related instructions cannot go into annulled delay > > +slots, it messes up the dwarf info. */ > > + if (RTX_FRAME_RELATED_P (trial)) > > + return; > > Don't you need to use break rather than return? I am not sure about this. There is an earlier if statement in the loop that does a 'return' instead of a break (or continue) and there is a return in the 'else' part of the if that sets must_annul. Both of these are inside the loop that looks at all the instructions in the sequence 'seq'. I think the code is looking at all the instructions in the sequence and if any of them fail one of the tests in the loop (in this case require annulling) then we can't handle any of the instructions in the sequence and so we return immediately without putting any of the instructions from 'seq' in the delay slot. I believe a frame related instruction in an annulled branch needs to be handled that way too. > > > + else if (!RTX_FRAME_RELATED_P (trial) \ > > Stray backslash. That is easily fixed. > Other than that I think this is OK. There are some preexisting tests for > frame related insns already in this code. > > > Bernd >
Re: [Patch] Fix bug for frame instructions in annulled delay slots
On 12/07/2015 08:43 PM, Steve Ellcey wrote: I am not sure about this. There is an earlier if statement in the loop that does a 'return' instead of a break (or continue) and there is a return in the 'else' part of the if that sets must_annul. Both of these are inside the loop that looks at all the instructions in the sequence 'seq'. I think the code is looking at all the instructions in the sequence and if any of them fail one of the tests in the loop (in this case require annulling) then we can't handle any of the instructions in the sequence and so we return immediately without putting any of the instructions from 'seq' in the delay slot. I believe a frame related instruction in an annulled branch needs to be handled that way too. Ah, I think I was looking at the other function that has the same must_annul test (steal_delay_list_from_fallthrough). The patch is ok without the backslash. Maybe the other function ought to be changed as well though? Bernd
Re: [Patch] Fix bug for frame instructions in annulled delay slots
On Mon, 2015-12-07 at 20:59 +0100, Bernd Schmidt wrote: > On 12/07/2015 08:43 PM, Steve Ellcey wrote: > > I am not sure about this. There is an earlier if statement in the loop > > that does a 'return' instead of a break (or continue) and there is a > > return in the 'else' part of the if that sets must_annul. Both of these > > are inside the loop that looks at all the instructions in the sequence > > 'seq'. I think the code is looking at all the instructions in the > > sequence and if any of them fail one of the tests in the loop (in this > > case require annulling) then we can't handle any of the instructions in > > the sequence and so we return immediately without putting any of the > > instructions from 'seq' in the delay slot. I believe a frame related > > instruction in an annulled branch needs to be handled that way too. > > Ah, I think I was looking at the other function that has the same > must_annul test (steal_delay_list_from_fallthrough). The patch is ok > without the backslash. Maybe the other function ought to be changed as > well though? > > > Bernd That would seem reasonable, though I don't have a test case for where the change to that routine would actually make a difference in the compilation. I'll create a patch and test it to make sure it doesn't cause any problems and then submit it as a follow-up to this change. Steve Ellcey sell...@imgtec.com
Re: [Patch] Fix bug for frame instructions in annulled delay slots
On Mon, 2015-12-07 at 12:30 -0700, Jeff Law wrote: > On 12/07/2015 12:28 PM, Bernd Schmidt wrote: > > On 12/07/2015 07:54 PM, Steve Ellcey wrote: > >> if (must_annul) > >> -used_annul = 1; > >> +{ > >> + /* Frame related instructions cannot go into annulled delay > >> + slots, it messes up the dwarf info. */ > >> + if (RTX_FRAME_RELATED_P (trial)) > >> +return; > > > > Don't you need to use break rather than return? > > > >> + else if (!RTX_FRAME_RELATED_P (trial) \ > > > > Stray backslash > > Other than that I think this is OK. There are some preexisting tests for > > frame related insns already in this code. > Also note there's probably port cleanup that could happen once this goes > in. IIRC the PA port (for example) explicitly disallows frame related > insns from many (most, all?) delay slots. Other targets may be doing > something similar. > > jeff If I had remembered/seen that code in pa.md earlier I might have fixed MIPS the same way. Oh well, I guess changing reorg.c is the better way to do it since it can be shared now. It looks like steal_delay_list_from_fallthrough is missing a lot of FRAME_RELATED checks (and other things) that steal_delay_list_from_target has in it (not just my changes). I think that means most of the work/fixing has been on machines like MIPS where the code in the delay slot is either always executed or only executed when the branch is taken (vs. untaken). I think steal_delay_list_from_fallthrough is only going to be used when the delay slot is executed on a branch-not-taken. Are there any machines like that? Steve Ellcey sell...@imgtec.com
[Patch] Fix bug for frame instructions in annulled delay slots
While doing more testing of the MIPS frame header optimization I checked in earlier, I found I could not build the elf toolchain when I turned the optimization on by default. This is because GCC was putting a frame related instruction (the save of the return address) into an annulled delay slot. This only happens when the function prologue is moved after a branch by the shrink wrap optimization and it caused create_cfi_notes to abort because the code that generates the CFI information cannot handle stack related annulled instructions. This never occurred when I did my testing with the linux toolchain because I did not use -Os (which caused the use of instructions with annulled delay slots) and because -O2 does more inlining than -Os which meant there were fewer places where the return address could be saved in the callers frame header without the callee having to allocate its own stack space. The fix for this is not in mips specific code but in reorg.c where we restrict GCC from putting any frame related instructions into annulled delay slots. There was already code in reorg.c to prevent frame related instructions from going into normal delay slots but that code did not seem to cover all the unnulled delay slot situations. This patch extends the code to not put frame related instructions into annulled delay slots. Tested on MIPS with linux and elf toolchain builds and testsuite runs. OK to checkin? Steve Ellcey sell...@imgtec.com 2015-12-07 Steve Ellcey* reorg.c (optimize_skip): Do not put frame related instructions in annulled delay slots. (steal_delay_list_from_target): Ditto. (fill_slots_from_thread): Ditto. diff --git a/gcc/reorg.c b/gcc/reorg.c index cc68d6b..bab5f49 100644 --- a/gcc/reorg.c +++ b/gcc/reorg.c @@ -739,6 +739,7 @@ optimize_skip (rtx_jump_insn *insn, vec *delay_list) || recog_memoized (trial) < 0 || (! eligible_for_annul_false (insn, 0, trial, flags) && ! eligible_for_annul_true (insn, 0, trial, flags)) + || RTX_FRAME_RELATED_P (trial) || can_throw_internal (trial)) return; @@ -1126,7 +1127,13 @@ steal_delay_list_from_target (rtx_insn *insn, rtx condition, rtx_sequence *seq, trial, flags))) { if (must_annul) - used_annul = 1; + { + /* Frame related instructions cannot go into annulled delay +slots, it messes up the dwarf info. */ + if (RTX_FRAME_RELATED_P (trial)) + return; + used_annul = 1; + } rtx_insn *temp = copy_delay_slot_insn (trial); INSN_FROM_TARGET_P (temp) = 1; add_to_delay_list (temp, _delay_list); @@ -2464,9 +2471,9 @@ fill_slots_from_thread (rtx_jump_insn *insn, rtx condition, if (eligible_for_delay (insn, *pslots_filled, trial, flags)) goto winner; } - else if (0 - || (ANNUL_IFTRUE_SLOTS && ! thread_if_true) - || (ANNUL_IFFALSE_SLOTS && thread_if_true)) + else if (!RTX_FRAME_RELATED_P (trial) \ + && ((ANNUL_IFTRUE_SLOTS && ! thread_if_true) + || (ANNUL_IFFALSE_SLOTS && thread_if_true))) { old_trial = trial; trial = try_split (pat, trial, 0); 2015-12-07 Steve Ellcey * gcc.target/mips/wrap-delay.c: New test. diff --git a/gcc/testsuite/gcc.target/mips/wrap-delay.c b/gcc/testsuite/gcc.target/mips/wrap-delay.c index e69de29..1332a68 100644 --- a/gcc/testsuite/gcc.target/mips/wrap-delay.c +++ b/gcc/testsuite/gcc.target/mips/wrap-delay.c @@ -0,0 +1,42 @@ +/* { dg-do compile } */ +/* { dg-options "-g -mframe-header-opt -mbranch-likely" } */ + +/* GCC was generating an ICE with the above options and -Os because + it was putting the save of $31 in two annulled delay slots but + there was only one restore since only one of the two saves could be + executed. This was correct code but it confused dwarf2cfi and + caused it to ICE when using the -g option. */ + + +enum demangle_component_type +{ + DEMANGLE_COMPONENT_TRINARY_ARG2, +}; +struct demangle_component +{ + enum demangle_component_type type; +}; +struct d_info +{ + int next_comp; + int num_comps; +}; +struct demangle_component * d_make_empty (struct d_info *di) +{ + if (di->next_comp >= di->num_comps) return ((void *)0); + ++di->next_comp; +} +struct demangle_component *d_make_comp ( + struct d_info *di, + enum demangle_component_type type, + struct demangle_component *left) +{ + struct demangle_component *p; + switch (type) +{ +case DEMANGLE_COMPONENT_TRINARY_ARG2: + if (left == ((void *)0)) return ((void *)0); +} + p = d_make_empty (di); + p->type = type; +}