| Message ID | 20150106140805.GK1667@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 1/6/2015 9:08 AM, Jakub Jelinek wrote: > @@ -2527,7 +2518,13 @@ scan_insn (bb_info_t bb_info, rtx_insn * > const_call ? "const" : "memset", INSN_UID (insn)); > > /* See the head comment of the frame_read field. */ > - if (reload_completed) > + if (reload_completed > + /* Tail calls are storing their arguments using > + arg poinnter. If it is a frame pointer on the target, Typo. > + even before reload we need to kill frame pointer based > + stores. */ > + || (SIBLING_CALL_P (insn) > + && HARD_FRAME_POINTER_IS_ARG_POINTER)) > insn_info->frame_read = true; > I had tested this hunk before, without the "HARD_FRAME_POINTER_IS_ARG_POINTER" addition, on 32-bit hppa and it resolved the original test case. Thanks, Dave
On Tue, Jan 06, 2015 at 12:07:17PM -0500, John David Anglin wrote: > On 1/6/2015 9:08 AM, Jakub Jelinek wrote: > >@@ -2527,7 +2518,13 @@ scan_insn (bb_info_t bb_info, rtx_insn * > > const_call ? "const" : "memset", INSN_UID (insn)); > > /* See the head comment of the frame_read field. */ > >- if (reload_completed) > >+ if (reload_completed > >+ /* Tail calls are storing their arguments using > >+ arg poinnter. If it is a frame pointer on the target, > Typo. Consider it fixed in my copy. > >+ even before reload we need to kill frame pointer based > >+ stores. */ > >+ || (SIBLING_CALL_P (insn) > >+ && HARD_FRAME_POINTER_IS_ARG_POINTER)) > > insn_info->frame_read = true; > > I had tested this hunk before, without the > "HARD_FRAME_POINTER_IS_ARG_POINTER" > addition, on 32-bit hppa and it resolved the original test case. Jakub
On 01/06/15 07:08, Jakub Jelinek wrote: > Hi! > > On Mon, Jan 05, 2015 at 10:31:17PM +0100, Jakub Jelinek wrote: >> Or you could e.g. do the >> if (HARD_FRAME_POINTER_IS_ARG_POINTER >> && !reload_completed >> && SIBLING_CALL_P (insn)) >> { add_wild_read (bb_info); return; } >> case first, then compute const_call and memset_call, >> if (const_call || memset_call) >> { current_big_block } >> else if (reload_completed && SIBLING_CALL_P (insn)) >> add_wild_read (bb_info); >> else >> add_non_frame_wild_read (bb_info); >> >> That way, you would not punish const/memset calls unnecessarily after reload >> when they are sibling calls. > > So what about this way? Even for the HARD_FRAME_POINTER_IS_ARG_POINTER > before reload case, there is no reason for wild read IMHO, just setting > frame_read should do all that is necessary. Bootstrapped/regtested on > x86_64-linux and i686-linux, tested on the pr55023 testcase with cross > to hppa-linux. Ok for trunk? > > 2015-01-06 Jakub Jelinek <jakub@redhat.com> > > PR target/55023 > PR middle-end/64388 > * dse.c (struct insn_info): Mention frame_read set also > before reload for tail calls on some targets. > (scan_insn): Revert 2014-12-22 change. Set frame_read > also before reload for tail calls if > HARD_FRAME_POINTER_IS_ARG_POINTER. Call add_wild_read > instead of add_non_frame_wild_read for non-const/memset > tail calls after reload. OK with the typo John pointed out fixed. jeff
--- gcc/dse.c.jj 2015-01-05 22:41:19.000000000 +0100 +++ gcc/dse.c 2015-01-06 11:39:39.450832915 +0100 @@ -371,9 +371,11 @@ struct insn_info either stack pointer or hard frame pointer based. This means that we have no other choice than also killing all the frame pointer based stores upon encountering a const function call. - This field is set after reload for const function calls. Having - this set is less severe than a wild read, it just means that all - the frame related stores are killed rather than all the stores. */ + This field is set after reload for const function calls and before + reload for const tail function calls on targets where arg pointer + is the frame pointer. Having this set is less severe than a wild + read, it just means that all the frame related stores are killed + rather than all the stores. */ bool frame_read; /* This field is only used for the processing of const functions. @@ -2483,17 +2485,6 @@ scan_insn (bb_info_t bb_info, rtx_insn * insn_info->cannot_delete = true; - /* Arguments for a sibling call that are pushed to memory are passed - using the incoming argument pointer of the current function. These - may or may not be frame related depending on the target. Since - argument pointer related stores are not currently tracked, we treat - a sibling call as though it does a wild read. */ - if (SIBLING_CALL_P (insn)) - { - add_wild_read (bb_info); - return; - } - /* Const functions cannot do anything bad i.e. read memory, however, they can read their parameters which may have been pushed onto the stack. @@ -2527,7 +2518,13 @@ scan_insn (bb_info_t bb_info, rtx_insn * const_call ? "const" : "memset", INSN_UID (insn)); /* See the head comment of the frame_read field. */ - if (reload_completed) + if (reload_completed + /* Tail calls are storing their arguments using + arg poinnter. If it is a frame pointer on the target, + even before reload we need to kill frame pointer based + stores. */ + || (SIBLING_CALL_P (insn) + && HARD_FRAME_POINTER_IS_ARG_POINTER)) insn_info->frame_read = true; /* Loop over the active stores and remove those which are @@ -2601,7 +2598,11 @@ scan_insn (bb_info_t bb_info, rtx_insn * } } } - + else if (SIBLING_CALL_P (insn) && reload_completed) + /* Arguments for a sibling call that are pushed to memory are passed + using the incoming argument pointer of the current function. After + reload that might be (and likely is) frame pointer based. */ + add_wild_read (bb_info); else /* Every other call, including pure functions, may read any memory that is not relative to the frame. */