| Message ID | CAMe9rOq-rrmXKUGExxGMb-AWYcVe94e4oL=egYPpnAt+LAOnOA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2015-01-03, at 3:18 PM, H.J. Lu wrote: > On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin <dave.anglin@bell.net> wrote: >> On 2015-01-03, at 2:48 PM, H.J. Lu wrote: >> >>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin >>> <dave@hiauly1.hia.nrc.ca> wrote: >>>> On Wed, 31 Dec 2014, H.J. Lu wrote: >>>> >>>>> - /* 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)) >>>>> + if (targetm.sibcall_wild_read_p (insn)) >>>>> { >>>>> add_wild_read (bb_info); >>>>> return; >>>> >>>> Instead of falling through to code designed to handle normal calls, it >>>> would be better to treat them separately. Potentially, there are other >>>> optimizations that may be applicable. If a sibcall doesn't read from >>>> the frame, add_non_frame_wild_read() can be called. This would restore >>>> the x86 optimization. >>>> >>> >>> That will a new optimization. I am trying to restore the old behavior on >>> x86 with minimum impact in stage 3. >> >> >> Not really. In gcc.dg/pr44194-1.c, the sibcall was not a const function and this case >> was covered by this hunk of code: >> >> else >> /* Every other call, including pure functions, may read any memory >> that is not relative to the frame. */ >> add_non_frame_wild_read (bb_info); >> > > Revision 219037 has > > diff --git a/gcc/dse.c b/gcc/dse.c > index 2555bd1..3a7f31c 100644 > --- a/gcc/dse.c > +++ b/gcc/dse.c > @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *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. > > My patch changes it to > > diff --git a/gcc/dse.c b/gcc/dse.c > index 2555bd1..c0e1a0c 100644 > --- a/gcc/dse.c > +++ b/gcc/dse.c > @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) > > insn_info->cannot_delete = true; > > + if (targetm.sibcall_wild_read_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. > > On x86, it is the same as before revision 219037 since > targetm.sibcall_wild_read_p always returns false on x86. Understood. The point is the subsequent code for const functions is based on assumptions that are not generally true for sibcalls: /* This field is only used for the processing of const functions. These functions cannot read memory, but they can read the stack because that is where they may get their parms. We need to be this conservative because, like the store motion pass, we don't consider CALL_INSN_FUNCTION_USAGE when processing call insns. Moreover, we need to distinguish two cases: 1. Before reload (register elimination), the stores related to outgoing arguments are stack pointer based and thus deemed of non-constant base in this pass. This requires special handling but also means that the frame pointer based stores need not be killed upon encountering a const function call. 2. After reload, the stores related to outgoing arguments can be 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. */ bool frame_read; For example, the stores related to sibcall arguments are not in general stack pointer based. This suggests to me that we don't have to always kill stack pointer based stores in the const sibcall case and they can be optimized. For me, keeping the sibcall handling separate from normal calls is easier to understand and potentially provides a means to optimize stack pointer based stores. Are you sure that the prior behaviour was always correct on x86 (e.g., more than 6 arguments)? Dave -- John David Anglin dave.anglin@bell.net
On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin <dave.anglin@bell.net> wrote: > On 2015-01-03, at 3:18 PM, H.J. Lu wrote: > >> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin <dave.anglin@bell.net> wrote: >>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote: >>> >>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin >>>> <dave@hiauly1.hia.nrc.ca> wrote: >>>>> On Wed, 31 Dec 2014, H.J. Lu wrote: >>>>> >>>>>> - /* 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)) >>>>>> + if (targetm.sibcall_wild_read_p (insn)) >>>>>> { >>>>>> add_wild_read (bb_info); >>>>>> return; >>>>> >>>>> Instead of falling through to code designed to handle normal calls, it >>>>> would be better to treat them separately. Potentially, there are other >>>>> optimizations that may be applicable. If a sibcall doesn't read from >>>>> the frame, add_non_frame_wild_read() can be called. This would restore >>>>> the x86 optimization. >>>>> >>>> >>>> That will a new optimization. I am trying to restore the old behavior on >>>> x86 with minimum impact in stage 3. >>> >>> >>> Not really. In gcc.dg/pr44194-1.c, the sibcall was not a const function and this case >>> was covered by this hunk of code: >>> >>> else >>> /* Every other call, including pure functions, may read any memory >>> that is not relative to the frame. */ >>> add_non_frame_wild_read (bb_info); >>> >> >> Revision 219037 has >> >> diff --git a/gcc/dse.c b/gcc/dse.c >> index 2555bd1..3a7f31c 100644 >> --- a/gcc/dse.c >> +++ b/gcc/dse.c >> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *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. >> >> My patch changes it to >> >> diff --git a/gcc/dse.c b/gcc/dse.c >> index 2555bd1..c0e1a0c 100644 >> --- a/gcc/dse.c >> +++ b/gcc/dse.c >> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) >> >> insn_info->cannot_delete = true; >> >> + if (targetm.sibcall_wild_read_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. >> >> On x86, it is the same as before revision 219037 since >> targetm.sibcall_wild_read_p always returns false on x86. > > > Understood. The point is the subsequent code for const functions is based on assumptions that > are not generally true for sibcalls: > > /* This field is only used for the processing of const functions. > These functions cannot read memory, but they can read the stack > because that is where they may get their parms. We need to be > this conservative because, like the store motion pass, we don't > consider CALL_INSN_FUNCTION_USAGE when processing call insns. > Moreover, we need to distinguish two cases: > 1. Before reload (register elimination), the stores related to > outgoing arguments are stack pointer based and thus deemed > of non-constant base in this pass. This requires special > handling but also means that the frame pointer based stores > need not be killed upon encountering a const function call. > 2. After reload, the stores related to outgoing arguments can be > 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. */ > bool frame_read; > > For example, the stores related to sibcall arguments are not in general stack pointer based. This > suggests to me that we don't have to always kill stack pointer based stores in the const sibcall case > and they can be optimized. > > For me, keeping the sibcall handling separate from normal calls is easier to understand and > potentially provides a means to optimize stack pointer based stores. Are you sure that the prior > behaviour was always correct on x86 (e.g., more than 6 arguments)? > I'd like to do it in 2 steps: 1. Bring x86 back to the behavior prior to revision 21903 since it won't cause any regressions. 2. Investigate if sibcall is handled correctly on x86.
On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote: >On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin ><dave.anglin@bell.net> wrote: >> On 2015-01-03, at 3:18 PM, H.J. Lu wrote: >> >>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin ><dave.anglin@bell.net> wrote: >>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote: >>>> >>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin >>>>> <dave@hiauly1.hia.nrc.ca> wrote: >>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote: >>>>>> >>>>>>> - /* 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)) >>>>>>> + if (targetm.sibcall_wild_read_p (insn)) >>>>>>> { >>>>>>> add_wild_read (bb_info); >>>>>>> return; >>>>>> >>>>>> Instead of falling through to code designed to handle normal >calls, it >>>>>> would be better to treat them separately. Potentially, there are >other >>>>>> optimizations that may be applicable. If a sibcall doesn't read >from >>>>>> the frame, add_non_frame_wild_read() can be called. This would >restore >>>>>> the x86 optimization. >>>>>> >>>>> >>>>> That will a new optimization. I am trying to restore the old >behavior on >>>>> x86 with minimum impact in stage 3. >>>> >>>> >>>> Not really. In gcc.dg/pr44194-1.c, the sibcall was not a const >function and this case >>>> was covered by this hunk of code: >>>> >>>> else >>>> /* Every other call, including pure functions, may read any >memory >>>> that is not relative to the frame. */ >>>> add_non_frame_wild_read (bb_info); >>>> >>> >>> Revision 219037 has >>> >>> diff --git a/gcc/dse.c b/gcc/dse.c >>> index 2555bd1..3a7f31c 100644 >>> --- a/gcc/dse.c >>> +++ b/gcc/dse.c >>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *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. >>> >>> My patch changes it to >>> >>> diff --git a/gcc/dse.c b/gcc/dse.c >>> index 2555bd1..c0e1a0c 100644 >>> --- a/gcc/dse.c >>> +++ b/gcc/dse.c >>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) >>> >>> insn_info->cannot_delete = true; >>> >>> + if (targetm.sibcall_wild_read_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. >>> >>> On x86, it is the same as before revision 219037 since >>> targetm.sibcall_wild_read_p always returns false on x86. >> >> >> Understood. The point is the subsequent code for const functions is >based on assumptions that >> are not generally true for sibcalls: >> >> /* This field is only used for the processing of const functions. >> These functions cannot read memory, but they can read the stack >> because that is where they may get their parms. We need to be >> this conservative because, like the store motion pass, we don't >> consider CALL_INSN_FUNCTION_USAGE when processing call insns. >> Moreover, we need to distinguish two cases: >> 1. Before reload (register elimination), the stores related to >> outgoing arguments are stack pointer based and thus deemed >> of non-constant base in this pass. This requires special >> handling but also means that the frame pointer based stores >> need not be killed upon encountering a const function call. >> 2. After reload, the stores related to outgoing arguments can be >> 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. >*/ >> bool frame_read; >> >> For example, the stores related to sibcall arguments are not in >general stack pointer based. This >> suggests to me that we don't have to always kill stack pointer based >stores in the const sibcall case >> and they can be optimized. >> >> For me, keeping the sibcall handling separate from normal calls is >easier to understand and >> potentially provides a means to optimize stack pointer based stores. >Are you sure that the prior >> behaviour was always correct on x86 (e.g., more than 6 arguments)? >> > >I'd like to do it in 2 steps: > >1. Bring x86 back to the behavior prior to revision 21903 since it >won't >cause any regressions. >2. Investigate if sibcall is handled correctly on x86. But either your new hook or the original fix makes no sense. Richard.
On Sun, Jan 4, 2015 at 3:37 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote: >>On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin >><dave.anglin@bell.net> wrote: >>> On 2015-01-03, at 3:18 PM, H.J. Lu wrote: >>> >>>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin >><dave.anglin@bell.net> wrote: >>>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote: >>>>> >>>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin >>>>>> <dave@hiauly1.hia.nrc.ca> wrote: >>>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote: >>>>>>> >>>>>>>> - /* 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)) >>>>>>>> + if (targetm.sibcall_wild_read_p (insn)) >>>>>>>> { >>>>>>>> add_wild_read (bb_info); >>>>>>>> return; >>>>>>> >>>>>>> Instead of falling through to code designed to handle normal >>calls, it >>>>>>> would be better to treat them separately. Potentially, there are >>other >>>>>>> optimizations that may be applicable. If a sibcall doesn't read >>from >>>>>>> the frame, add_non_frame_wild_read() can be called. This would >>restore >>>>>>> the x86 optimization. >>>>>>> >>>>>> >>>>>> That will a new optimization. I am trying to restore the old >>behavior on >>>>>> x86 with minimum impact in stage 3. >>>>> >>>>> >>>>> Not really. In gcc.dg/pr44194-1.c, the sibcall was not a const >>function and this case >>>>> was covered by this hunk of code: >>>>> >>>>> else >>>>> /* Every other call, including pure functions, may read any >>memory >>>>> that is not relative to the frame. */ >>>>> add_non_frame_wild_read (bb_info); >>>>> >>>> >>>> Revision 219037 has >>>> >>>> diff --git a/gcc/dse.c b/gcc/dse.c >>>> index 2555bd1..3a7f31c 100644 >>>> --- a/gcc/dse.c >>>> +++ b/gcc/dse.c >>>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *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. >>>> >>>> My patch changes it to >>>> >>>> diff --git a/gcc/dse.c b/gcc/dse.c >>>> index 2555bd1..c0e1a0c 100644 >>>> --- a/gcc/dse.c >>>> +++ b/gcc/dse.c >>>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) >>>> >>>> insn_info->cannot_delete = true; >>>> >>>> + if (targetm.sibcall_wild_read_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. >>>> >>>> On x86, it is the same as before revision 219037 since >>>> targetm.sibcall_wild_read_p always returns false on x86. >>> >>> >>> Understood. The point is the subsequent code for const functions is >>based on assumptions that >>> are not generally true for sibcalls: >>> >>> /* This field is only used for the processing of const functions. >>> These functions cannot read memory, but they can read the stack >>> because that is where they may get their parms. We need to be >>> this conservative because, like the store motion pass, we don't >>> consider CALL_INSN_FUNCTION_USAGE when processing call insns. >>> Moreover, we need to distinguish two cases: >>> 1. Before reload (register elimination), the stores related to >>> outgoing arguments are stack pointer based and thus deemed >>> of non-constant base in this pass. This requires special >>> handling but also means that the frame pointer based stores >>> need not be killed upon encountering a const function call. >>> 2. After reload, the stores related to outgoing arguments can be >>> 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. >>*/ >>> bool frame_read; >>> >>> For example, the stores related to sibcall arguments are not in >>general stack pointer based. This >>> suggests to me that we don't have to always kill stack pointer based >>stores in the const sibcall case >>> and they can be optimized. >>> >>> For me, keeping the sibcall handling separate from normal calls is >>easier to understand and >>> potentially provides a means to optimize stack pointer based stores. >>Are you sure that the prior >>> behaviour was always correct on x86 (e.g., more than 6 arguments)? >>> >> >>I'd like to do it in 2 steps: >> >>1. Bring x86 back to the behavior prior to revision 21903 since it >>won't >>cause any regressions. >>2. Investigate if sibcall is handled correctly on x86. > > But either your new hook or the original fix makes no sense. All I want is to restore the old behavior on x86. If the original fix makes no sense, should it be reverted?
On January 4, 2015 3:57:07 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote: >On Sun, Jan 4, 2015 at 3:37 AM, Richard Biener ><richard.guenther@gmail.com> wrote: >> On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com> >wrote: >>>On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin >>><dave.anglin@bell.net> wrote: >>>> On 2015-01-03, at 3:18 PM, H.J. Lu wrote: >>>> >>>>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin >>><dave.anglin@bell.net> wrote: >>>>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote: >>>>>> >>>>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin >>>>>>> <dave@hiauly1.hia.nrc.ca> wrote: >>>>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote: >>>>>>>> >>>>>>>>> - /* 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)) >>>>>>>>> + if (targetm.sibcall_wild_read_p (insn)) >>>>>>>>> { >>>>>>>>> add_wild_read (bb_info); >>>>>>>>> return; >>>>>>>> >>>>>>>> Instead of falling through to code designed to handle normal >>>calls, it >>>>>>>> would be better to treat them separately. Potentially, there >are >>>other >>>>>>>> optimizations that may be applicable. If a sibcall doesn't >read >>>from >>>>>>>> the frame, add_non_frame_wild_read() can be called. This would >>>restore >>>>>>>> the x86 optimization. >>>>>>>> >>>>>>> >>>>>>> That will a new optimization. I am trying to restore the old >>>behavior on >>>>>>> x86 with minimum impact in stage 3. >>>>>> >>>>>> >>>>>> Not really. In gcc.dg/pr44194-1.c, the sibcall was not a const >>>function and this case >>>>>> was covered by this hunk of code: >>>>>> >>>>>> else >>>>>> /* Every other call, including pure functions, may read >any >>>memory >>>>>> that is not relative to the frame. */ >>>>>> add_non_frame_wild_read (bb_info); >>>>>> >>>>> >>>>> Revision 219037 has >>>>> >>>>> diff --git a/gcc/dse.c b/gcc/dse.c >>>>> index 2555bd1..3a7f31c 100644 >>>>> --- a/gcc/dse.c >>>>> +++ b/gcc/dse.c >>>>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn >*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. >>>>> >>>>> My patch changes it to >>>>> >>>>> diff --git a/gcc/dse.c b/gcc/dse.c >>>>> index 2555bd1..c0e1a0c 100644 >>>>> --- a/gcc/dse.c >>>>> +++ b/gcc/dse.c >>>>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn >*insn) >>>>> >>>>> insn_info->cannot_delete = true; >>>>> >>>>> + if (targetm.sibcall_wild_read_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. >>>>> >>>>> On x86, it is the same as before revision 219037 since >>>>> targetm.sibcall_wild_read_p always returns false on x86. >>>> >>>> >>>> Understood. The point is the subsequent code for const functions >is >>>based on assumptions that >>>> are not generally true for sibcalls: >>>> >>>> /* This field is only used for the processing of const functions. >>>> These functions cannot read memory, but they can read the >stack >>>> because that is where they may get their parms. We need to be >>>> this conservative because, like the store motion pass, we >don't >>>> consider CALL_INSN_FUNCTION_USAGE when processing call insns. >>>> Moreover, we need to distinguish two cases: >>>> 1. Before reload (register elimination), the stores related to >>>> outgoing arguments are stack pointer based and thus deemed >>>> of non-constant base in this pass. This requires special >>>> handling but also means that the frame pointer based stores >>>> need not be killed upon encountering a const function call. >>>> 2. After reload, the stores related to outgoing arguments can >be >>>> 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. >>>*/ >>>> bool frame_read; >>>> >>>> For example, the stores related to sibcall arguments are not in >>>general stack pointer based. This >>>> suggests to me that we don't have to always kill stack pointer >based >>>stores in the const sibcall case >>>> and they can be optimized. >>>> >>>> For me, keeping the sibcall handling separate from normal calls is >>>easier to understand and >>>> potentially provides a means to optimize stack pointer based >stores. >>>Are you sure that the prior >>>> behaviour was always correct on x86 (e.g., more than 6 arguments)? >>>> >>> >>>I'd like to do it in 2 steps: >>> >>>1. Bring x86 back to the behavior prior to revision 21903 since it >>>won't >>>cause any regressions. >>>2. Investigate if sibcall is handled correctly on x86. >> >> But either your new hook or the original fix makes no sense. > >All I want is to restore the old behavior on x86. If the original fix >makes no sense, should it be reverted? I think the original fix is too conservative But I also think there is a target independent bug, thus a target hook to "fix" things for x86 makes no sense. Richard.
On 01/04/15 10:16, Richard Biener wrote: >>> >>> But either your new hook or the original fix makes no sense. >> >> All I want is to restore the old behavior on x86. If the original fix >> makes no sense, should it be reverted? > > I think the original fix is too conservative Perhaps. Neither John nor I felt it was too conservative. But disagreeing slightly on this isn't a big deal. > But I also think there is a target independent bug, thus a target hook to "fix" things for x86 makes no sense. Agreed 100%. Jeff
On Mon, Jan 05, 2015 at 07:19:33AM -0700, Jeff Law wrote: > On 01/04/15 10:16, Richard Biener wrote: > >>> > >>>But either your new hook or the original fix makes no sense. > >> > >>All I want is to restore the old behavior on x86. If the original fix > >>makes no sense, should it be reverted? > > > >I think the original fix is too conservative > Perhaps. Neither John nor I felt it was too conservative. But disagreeing > slightly on this isn't a big deal. > > >But I also think there is a target independent bug, thus a target hook to "fix" things for x86 makes no sense. > Agreed 100%. So isn't the right replacement for the target hook H.J. wanted to add the HARD_FRAME_POINTER_IS_ARG_POINTER macro? If argp is not hfp, then the stores through argp are not considered to be based on the frame pointer, if it is the same thing, such as on PA, then indeed sibcall stack argument stores are frame related. You could also add a !reload_completed to that, after RA frame_read is how this is handled. /* Arguments for a sibling call that are pushed to memory are passed using the incoming argument pointer of the current function. If argument pointer is the hard frame pointer, then treat sibling calls as wild reads before reload; after reload frame_read flag should take care of this. */ if (HARD_FRAME_POINTER_IS_ARG_POINTER && SIBLING_CALL_P (insn) && !reload_completed) { add_wild_read (bb_info); return; } ? Jakub
On 1/5/2015 1:51 PM, Jakub Jelinek wrote: > So isn't the right replacement for the target hook H.J. wanted to add > the HARD_FRAME_POINTER_IS_ARG_POINTER macro? > If argp is not hfp, then the stores through argp are not considered > to be based on the frame pointer, if it is the same thing, such as on PA, > then indeed sibcall stack argument stores are frame related. > You could also add a !reload_completed to that, after RA frame_read is > how this is handled. > > /* Arguments for a sibling call that are pushed to memory are passed > using the incoming argument pointer of the current function. If > argument pointer is the hard frame pointer, then treat sibling > calls as wild reads before reload; after reload frame_read flag > should take care of this. */ > if (HARD_FRAME_POINTER_IS_ARG_POINTER > && SIBLING_CALL_P (insn) > && !reload_completed) > { > add_wild_read (bb_info); > return; > } > > ? I think there may be one situation after reload that's not handled by the above. frame_read is only used for const calls. What about the situation where we have a non const sibcall and the frame pointer isn't eliminated? Dave
On Mon, Jan 05, 2015 at 03:16:46PM -0500, John David Anglin wrote: > I think there may be one situation after reload that's not handled > by the above. frame_read is only used for const calls. What about > the situation where we have a non const sibcall and the frame pointer > isn't eliminated? After reload DSE is run after threading prologues/epilogues, end the prologue/epilogue sequences usually contain wild reads, e.g. (mem:BLK (scratch)) in some insn etc. Do you have some particular testcase in mind? That said, DSE after reload is much more limited than the DSE before reload, so is less important, so perhaps even if ((HARD_FRAME_POINTER_IS_ARG_POINTER || reload_completed) && SIBLING_CALL_P (insn)) { add_wild_read (bb_info); return; } might be good enough. Jakub
On Mon, Jan 05, 2015 at 10:23:57PM +0100, Jakub Jelinek wrote: > On Mon, Jan 05, 2015 at 03:16:46PM -0500, John David Anglin wrote: > > I think there may be one situation after reload that's not handled > > by the above. frame_read is only used for const calls. What about > > the situation where we have a non const sibcall and the frame pointer > > isn't eliminated? > > After reload DSE is run after threading prologues/epilogues, end > the prologue/epilogue sequences usually contain wild reads, e.g. > (mem:BLK (scratch)) in some insn etc. > Do you have some particular testcase in mind? > > That said, DSE after reload is much more limited than the DSE before reload, > so is less important, so perhaps even > if ((HARD_FRAME_POINTER_IS_ARG_POINTER || reload_completed) > && SIBLING_CALL_P (insn)) > { > add_wild_read (bb_info); > return; > } > might be good enough. 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. Jakub
diff --git a/gcc/dse.c b/gcc/dse.c index 2555bd1..3a7f31c 100644 --- a/gcc/dse.c +++ b/gcc/dse.c @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *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. My patch changes it to diff --git a/gcc/dse.c b/gcc/dse.c index 2555bd1..c0e1a0c 100644 --- a/gcc/dse.c +++ b/gcc/dse.c @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) insn_info->cannot_delete = true; + if (targetm.sibcall_wild_read_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.