| Message ID | 201105241342.11824.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 24, 2011 at 01:42:11PM +0200, Eric Botcazou wrote: > > That is so that the argp (or framep) will be properly handled during > > CSELIB, as constant VALUE that is never changed through the function. And > > additionally to find out if we can do the replacement (we can't if argp > > resp. framep isn't eliminated and could therefore occur in the real code). > > OK, thanks for the explanation. So, would you like me to redo my patch on top of your patch, test it and submit? > > This comment is not 100% accurate. The reason we do that is to use the > > much more compact DW_OP_fbreg* if possible compared to huge location lists, > > especially for !frame_pointer_needed where the location list would need to > > have another entry whenever sp changes. dwarf2out.c rewrites argp/framep > > if eliminated into DW_OP_fbreg (with some offset when needed). > > Adjusted. Ok. > > While I had part of this in my patch too, it is unnecessary, if prologue_bb > > is set to non-NULL only when we want to use it, then we know fp_cfa_offset > > is not -1. By computing prologue_bb always, it will call fp_setter > > unnecessarily for every insn in the prologue bb for !frame_pointer_needed > > or for DRAP. I guess adding a comment instead of this would be better. > > The fp_cfa_offset != -1 was before the call to fp_setter in my first patch. :-) If you think it makes the code clearer, the extra comparison for at most each stmt in the prologue_bb is probably noise compile time wise. > > Revised version attached. Fine with me, but I'm not a reviewer of this part of GCC... Jakub
> So, would you like me to redo my patch on top of your patch, test it and > submit? I'll take care of that. > If you think it makes the code clearer, the extra comparison for at most > each stmt in the prologue_bb is probably noise compile time wise. No doubt about that. Plus this may save the next hacker a dozen of minutes trying to understand why a seemingly innocuous code movement yields unexpected differences in the debug info... > Fine with me, but I'm not a reviewer of this part of GCC... Yes, but there is none officially, you wrote the code and the patch only tweaks comments and makes a minor code change. Let's pretend that it's obvious. Tested on i586-suse-linux, applied on the mainline. Thanks again.
Index: var-tracking.c =================================================================== --- var-tracking.c (revision 174058) +++ var-tracking.c (working copy) @@ -705,7 +705,8 @@ vt_stack_adjustments (void) static rtx cfa_base_rtx; static HOST_WIDE_INT cfa_base_offset; -/* Compute a CFA-based value for the stack pointer. */ +/* Compute a CFA-based value for an ADJUSTMENT made to stack_pointer_rtx + or hard_frame_pointer_rtx. */ static inline rtx compute_cfa_pointer (HOST_WIDE_INT adjustment) @@ -8664,7 +8665,7 @@ vt_init_cfa_base (void) static bool vt_initialize (void) { - basic_block bb, prologue_bb = NULL; + basic_block bb, prologue_bb = single_succ (ENTRY_BLOCK_PTR); HOST_WIDE_INT fp_cfa_offset = -1; alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def)); @@ -8722,6 +8723,16 @@ vt_initialize (void) CLEAR_HARD_REG_SET (argument_reg_set); + /* In order to factor out the adjustments made to the stack pointer or to + the hard frame pointer and thus be able to use DW_OP_fbreg operations + instead of individual location lists, we're going to rewrite MEMs based + on them into MEMs based on the CFA by de-eliminating stack_pointer_rtx + or hard_frame_pointer_rtx to the virtual CFA pointer frame_pointer_rtx + resp. arg_pointer_rtx. We can do this either when there is no frame + pointer in the function and stack adjustments are consistent for all + basic blocks or when there is a frame pointer and no stack realignment. + But we first have to check that frame_pointer_rtx resp. arg_pointer_rtx + has been eliminated. */ if (!frame_pointer_needed) { rtx reg, elim; @@ -8764,10 +8775,11 @@ vt_initialize (void) } if (elim != hard_frame_pointer_rtx) fp_cfa_offset = -1; - else - prologue_bb = single_succ (ENTRY_BLOCK_PTR); } + else + fp_cfa_offset = -1; } + if (frame_pointer_needed) { rtx insn; @@ -8867,7 +8879,8 @@ vt_initialize (void) VTI (bb)->out.stack_adjust += post; } - if (bb == prologue_bb + if (fp_cfa_offset != -1 + && bb == prologue_bb && hard_frame_pointer_adjustment == -1 && RTX_FRAME_RELATED_P (insn) && fp_setter (insn))