[ARC] Refurbish emitting DWARF2 for epilogue.

Properly emit DWARF2 related information while expanding epilogue. Remove
the -m[no]-epilogue-cfi option as it is not needed any longer. This patch
solves the dwarf2cfi errors observed while running dejagnu tests.

Ok to commit?
Claudiu

gcc/
2015-11-27  Claudiu Zissulescu  <claziss@synopsys.com>

	* config/arc/arc.c (frame_move): Set frame related flag.
	(arc_save_restore): Emit epilogue related DWARF2 information.
	(arc_expand_epilogue): Likewise.
	* config/arc/arc.opt (mno-epilogue-cfi): Remove.
	(mepilogue-cfi): Likewise.
	* doc/invoke.texi: Remove -m[no]-epilogue-cfi documentation.
---
 gcc/config/arc/arc.c   | 87 ++++++++++++++++++++++++++++++++++++++++----------
 gcc/config/arc/arc.opt |  8 -----
 gcc/doc/invoke.texi    | 10 +-----
 3 files changed, 71 insertions(+), 34 deletions(-)

On 27/11/15 13:53, Claudiu Zissulescu wrote:
>    
> @@ -2502,11 +2540,18 @@ arc_expand_epilogue (int sibcall_p)
>     /* Restore any saved registers.  */
>     if (frame_pointer_needed)
>       {
> -	  rtx addr = gen_rtx_POST_INC (Pmode, stack_pointer_rtx);
> +      insn = emit_insn (gen_blockage ());

Is this actually part of the patch to fix cfi generation?
It looks to me like it is working around an alias.c issue - namely, that 
alias.c does
not consider stack and frame pointer based addresses to alias.
https://gcc.gnu.org/ml/gcc/2011-07/msg00461.html
https://github.com/adapteva/epiphany-gcc/commit/6d1194a563e05dfa826ab4635514477af1f7a2b0

Hi Joern,

> > +      insn = emit_insn (gen_blockage ());
> 
> Is this actually part of the patch to fix cfi generation?

This instruction prevents the delay branch scheduler to speculatively use epilogue instructions to fill up the delay slots. Hence, generating an assert during dwarf2cfi execution. This behavior is experiment by dg.exp/pr49994-1.s test.

At a closer inspection of the patch, I've refurbish it in a more generic fashion (attached), where the blockage guards the entire expand epilogue process.

It may be questionable if emitting blockage in epilogue is part of the cfi refactoring of the epilogue. However, without it we may still get errors in dwarf2cfi.

Thanks,
Claudiu

On 07/12/15 09:19, Claudiu Zissulescu wrote:
> Hi Joern,
>
>>> +      insn = emit_insn (gen_blockage ());
>> Is this actually part of the patch to fix cfi generation?
> This instruction prevents the delay branch scheduler to speculatively use epilogue instructions to fill up the delay slots. Hence, generating an assert during dwarf2cfi execution. This behavior is experiment by dg.exp/pr49994-1.s test.

The main point of having rtl epilogues is to allow such optimizations.
Traditionally, we have said that at -O1, it is OK to curb optimizations 
for the sake of having programs
that are saner to debug, while -O2 and above should just optimize, and 
the debug info generation is
just thrown in afterwards as a best-effort attempt.
Additionally, we also have -Og now.

You can also consider having separate options to control optimizations 
that affect debugging.
If leaving out epilogue cfi is what it takes to allow epilogue 
scheduling without the compiler crashing,
then that is what should be done by default at -O2, but if someone finds 
that particularly vexing,
they might appreciate a -mepilogue-cfi option to change that default, 
and instead disable some
scheduling (delay slot and otherwise).

On 09/12/15 04:43, Joern Wolfgang Rennecke wrote:
>
> You can also consider having separate options to control optimizations 
> that affect debugging.
> If leaving out epilogue cfi is what it takes to allow epilogue 
> scheduling without the compiler crashing,
> then that is what should be done by default at -O2, but if someone 
> finds that particularly vexing,
> they might appreciate a -mepilogue-cfi option to change that default, 
> and instead disable some
> scheduling (delay slot and otherwise).

P.S.: It might also make sense to build some or all libraries with such 
an option, depending on what
the priorities for performance / debuggability are.

> The main point of having rtl epilogues is to allow such optimizations.
> Traditionally, we have said that at -O1, it is OK to curb optimizations for the
> sake of having programs that are saner to debug, while -O2 and above should
> just optimize, and the debug info generation is just thrown in afterwards as a
> best-effort attempt.
> 
> Additionally, we also have -Og now.

Well, it seems to me that we prefer to disable optimizations when talking about debug related information (see PR target/60598 git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@208749 138bc75d-0d04-0410-961f-82ee72b054a4 commit).  

> 
> You can also consider having separate options to control optimizations that
> affect debugging.
> If leaving out epilogue cfi is what it takes to allow epilogue scheduling
> without the compiler crashing, then that is what should be done by default at
> -O2, but if someone finds that particularly vexing, they might appreciate a -
> mepilogue-cfi option to change that default, and instead disable some
> scheduling (delay slot and otherwise).

Unfortunately, dwarf2cfi checks the paths for consistency (dwarf2cfi.c:2284), throwing an error if those paths are not ok. Also, with ARC gcc we focus on Linux type of application, hence, the unwinding info needs to be consistent.  
As far as I can see, having a sort of mno-epilogue-cfi option will just inhibit or not the emitting of the blockage instruction, and the option will be valid only when compiling without emitting dwarf information (i.e., the mno-epilogue-cfi is incompatible with -g). 
Personally, I do not see the benefit of having such an option, as one may lose like 1 cycle per function call (HS/EM cpus) in very particular cases. Running the dg.exp, compile.exp, execute.exp, and builing Linux with some default apps, we found only 4 cases in which the branch scheduler slot needs the blockage mechanism. Also, adding blockage before generating any prologue instruction seems to be a wide spread practice in gcc backends (see SH for example). 

Best,
Claudiu

On 09/12/15 15:34, Claudiu Zissulescu wrote:

> Well, it seems to me that we prefer to disable optimizations when talking about debug related information (see PR target/60598 git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@208749 138bc75d-0d04-0410-961f-82ee72b054a4 commit).
Actually, unwind information might also be needed for exception 
handling, depending on the target.
(sjlj exception handling or dwarf2)
In which case it becomes a matter of program correctness if any 
exception might be raised that
could lead to incorrect unwinding due to incorrect/incomplete unwind info.

So were both wrong: this is not merely about debugging.

OTOH, the example you give also shows a much more nuanced approach to 
throttling optimization:
the patch doesn't dead all epilogue scheduling, but specifically tests 
for the presence of a frame related
insn at the point where it could cause trouble.
The equivalent would be to reject a frame related insn in the delay slot 
of a conditional branch.
For arc.md, that would likely involve splitting four of the define_delay 
statements into two each.
If that seems a bit much, a compromize would be to patch in_delay_slot, 
thus affecting unconditional branches too.
More ambitious approaches would be to move the note to a (zero-sized, 
pseudo-)placeholder insn, or
finding a way to make the dwarf2cfi cope with speculated frame related 
insns.  Eg.g even if the delay slot
is not anulled, could we mark the note as anulled?

Another approach would be to fix fill_eager_delay_slots not to stuff 
unconditional frame related insns into
non-anulled delay slots of conditional branches.
Or have some target hook to make it not even bother filling delay slots 
speculatively; for targets that can
fully unexpose the delay slot, like SH and ARC >= ARC700, this aspect of 
fill_eager_delay_slots only mucks up
schedules and increases code size.

> Unfortunately, dwarf2cfi checks the paths for consistency (dwarf2cfi.c:2284), throwing an error if those paths are not ok. Also, with ARC gcc we focus on Linux type of application, hence, the unwinding info needs to be consistent.
> As far as I can see, having a sort of mno-epilogue-cfi option will just inhibit or not the emitting of the blockage instruction, and the option will be valid only when compiling without emitting dwarf information (i.e., the mno-epilogue-cfi is incompatible with -g).
> Personally, I do not see the benefit of having such an option, as one may lose like 1 cycle per function call (HS/EM cpus) in very particular cases. Running the dg.exp, compile.exp, execute.exp, and builing Linux with some default apps, we found only 4 cases in which the branch scheduler slot needs the blockage mechanism.

The number of problems you found needs not bear any relation to the 
number of optimizations
suppressed by the blockage instruction.  Consider the case when there 
are high-latency unconditional
instructions just before a lengthy epilogue.  sched2 scheduling some 
epilogue instructions into the scheduling bubbles can hide the latencies.
More relevant ways to get data would be comparing the object files (from 
a whole toolchain library set and/or one or more big application(s)) 
built with/without the blockage insn emitted, or to benchmark it.
>   Also, adding blockage before generating any prologue instruction seems to be a wide spread practice in gcc backends (see SH for example).
It is a quick way to 'fix' (actually, hide) bugs.  At the expense of 
optimization.
It can be justified if the resources to keep a port in working order are 
limited, and so are the forgone
optimization benefits.
But at a minimum, you should add a comment to explain what problem you 
are papering over.
That works best if you actually file a bug report in bugzilla first 
(about the interaction of fill_eager_delay_slots and dwarf2cfi) so that 
you can name the bug.

> 
> OTOH, the example you give also shows a much more nuanced approach to
> throttling optimization:
> the patch doesn't dead all epilogue scheduling, but specifically tests for the
> presence of a frame related insn at the point where it could cause trouble.

Actually, in my revised patch I do move the blockage instruction earlier on, before emitting any epilogue related instruction.

> Or have some target hook to make it not even bother filling delay slots
> speculatively; for targets that can fully unexpose the delay slot, like SH and
> ARC >= ARC700, this aspect of fill_eager_delay_slots only mucks up
> schedules and increases code size.
> 
GCC has such a target:

bool TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P (void) [Target Hook]
This predicate controls the use of the eager delay slot filler to disallow speculatively
executed instructions being placed in delay slots. Targets such as certain MIPS architectures
possess both branches with and without delay slots. As the eager delay slot
filler can decrease performance, disabling it is beneficial when ordinary branches are
available. Use of delay slot branches filled using the basic filler is often still desirable
as the delay slot can hide a pipeline bubble.

We can enable this hook for ARC and see what are the numbers. Initially, I thought that we may lose more by enabling this hook. What do u say?

//Claudiu

H Joern,

> Or have some target hook to make it not even bother filling delay slots
> speculatively; for targets that can fully unexpose the delay slot, like SH and
> ARC >= ARC700, this aspect of fill_eager_delay_slots only mucks up
> schedules and increases code size.

I propose to solve the dwarf2 issues during epilogue by using the TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P hook for ARC processors. Hence, we do not need to emit the blockage instruction during epilogue. So, I have refactor the patch in two patches as follows:
	- The 0001-Refurbish-emitting-DWARF2-related-information-when-e.patch is the initial patch without emitting the blockage instruction during epilogue.
	- The 0002-ARC-Use-TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P-hook.patch adds TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P hook for ARC.

Both patches are attached.

> 
> More relevant ways to get data would be comparing the object files (from a
> whole toolchain library set and/or one or more big application(s)) built
> with/without the blockage insn emitted, or to benchmark it.

I did some testing here. For size, I used CSiBE testbench, and for speed, I used coremark and dhrystone. Using a blockage or not, doesn't affect the size or speed figures. However, using TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P hook betters the size figures (not much, just .1%), and improves the coremark by 2% and Dhrystone by 1%.

Hence, in the light of the new figures, I favor the above two patch solution. Both patches are checked using dg.exp and compile.exp. Ok to submit?

Thanks,
Claudiu

On 11/12/15 10:29, Claudiu Zissulescu wrote:
> I did some testing here. For size, I used CSiBE testbench, and for speed, I used coremark and dhrystone. Using a blockage or not, doesn't affect the size or speed figures. However, using TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P hook betters the size figures (not much, just .1%), and improves the coremark by 2% and Dhrystone by 1%.
>
> Hence, in the light of the new figures, I favor the above two patch solution. Both patches are checked using dg.exp and compile.exp. Ok to submit?
Both patches are OK.

Patches submitted.

Thanks,
Claudiu

> -----Original Message-----
> From: Joern Wolfgang Rennecke [mailto:gnu@amylaar.uk]
> Sent: Saturday, December 12, 2015 3:49 PM
> To: Claudiu Zissulescu; gcc-patches@gcc.gnu.org
> Cc: Francois.Bedard@synopsys.com; jeremy.bennett@embecosm.com
> Subject: Re: [PATCH][ARC] Refurbish emitting DWARF2 for epilogue.
> 
> 
> 
> On 11/12/15 10:29, Claudiu Zissulescu wrote:
> > I did some testing here. For size, I used CSiBE testbench, and for speed, I
> used coremark and dhrystone. Using a blockage or not, doesn't affect the
> size or speed figures. However, using
> TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P hook betters the size
> figures (not much, just .1%), and improves the coremark by 2% and
> Dhrystone by 1%.
> >
> > Hence, in the light of the new figures, I favor the above two patch solution.
> Both patches are checked using dg.exp and compile.exp. Ok to submit?
> Both patches are OK.