| Message ID | 20131118102208.GH21297@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 11/18/13 03:22, Ilya Enkovich wrote: > Hi, > > Here is a patch to disable splitting when bounds transfer is required for splitted function. > > Thanks, > Ilya > -- > 2013-11-13 Ilya Enkovich <ilya.enkovich@intel.com> > > * ipa-split.c: Include tree-chkp.h. > (consider_split): Do not split when splitted > part needs bounds transfer. ?!? Is there some fundamental reason why this shouldn't "just work"? I think more explanation is needed so we can evaluate this patch. jeff
2013/11/18 Jeff Law <law@redhat.com>: > On 11/18/13 03:22, Ilya Enkovich wrote: >> >> Hi, >> >> Here is a patch to disable splitting when bounds transfer is required for >> splitted function. >> >> Thanks, >> Ilya >> -- >> 2013-11-13 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * ipa-split.c: Include tree-chkp.h. >> (consider_split): Do not split when splitted >> part needs bounds transfer. > > ?!? Is there some fundamental reason why this shouldn't "just work"? I > think more explanation is needed so we can evaluate this patch. > > jeff > Fundamental reason here is that we do not pass standalone bounds to functions. Bounds are always passed in addition to pointer. Ilya
On 11/18/13 11:17, Ilya Enkovich wrote: > 2013/11/18 Jeff Law <law@redhat.com>: >> On 11/18/13 03:22, Ilya Enkovich wrote: >>> >>> Hi, >>> >>> Here is a patch to disable splitting when bounds transfer is required for >>> splitted function. >>> >>> Thanks, >>> Ilya >>> -- >>> 2013-11-13 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * ipa-split.c: Include tree-chkp.h. >>> (consider_split): Do not split when splitted >>> part needs bounds transfer. >> >> ?!? Is there some fundamental reason why this shouldn't "just work"? I >> think more explanation is needed so we can evaluate this patch. >> >> jeff >> > Fundamental reason here is that we do not pass standalone bounds to > functions. Bounds are always passed in addition to pointer. Right, but isn't all that information available to both the caller and callee in this case? jeff
2013/11/18 Jeff Law <law@redhat.com>: > On 11/18/13 11:17, Ilya Enkovich wrote: >> >> 2013/11/18 Jeff Law <law@redhat.com>: >>> >>> On 11/18/13 03:22, Ilya Enkovich wrote: >>>> >>>> >>>> Hi, >>>> >>>> Here is a patch to disable splitting when bounds transfer is required >>>> for >>>> splitted function. >>>> >>>> Thanks, >>>> Ilya >>>> -- >>>> 2013-11-13 Ilya Enkovich <ilya.enkovich@intel.com> >>>> >>>> * ipa-split.c: Include tree-chkp.h. >>>> (consider_split): Do not split when splitted >>>> part needs bounds transfer. >>> >>> >>> ?!? Is there some fundamental reason why this shouldn't "just work"? I >>> think more explanation is needed so we can evaluate this patch. >>> >>> jeff >>> >> Fundamental reason here is that we do not pass standalone bounds to >> functions. Bounds are always passed in addition to pointer. > > Right, but isn't all that information available to both the caller and > callee in this case? How does pointer passed to regular function differ from pointer passed to splitted function? How do I know then which pointer is to be passed with bounds and wchich one is not? Moreover current ABI does not allow to pass bounds with no pointer or pass bounds for some pointers in the call only. Ilya > > jeff
On 11/18/13 11:27, Ilya Enkovich wrote: > > How does pointer passed to regular function differ from pointer passed > to splitted function? How do I know then which pointer is to be passed > with bounds and wchich one is not? Moreover current ABI does not allow > to pass bounds with no pointer or pass bounds for some pointers in the > call only. But I don't see any case in function splitting where we're going to want to pass the pointer without the bounds. If you want the former, you're going to want the latter. I really don't see why you need to do anything special here. At the most an assert in the splitting code to ensure that you don't have a situation where there's mixed pointers with bounds and pointers without bounds should be all you need or that you passed a bounds with no associated pointer :-) Jeff
2013/11/18 Jeff Law <law@redhat.com>: > On 11/18/13 11:27, Ilya Enkovich wrote: >> >> >> How does pointer passed to regular function differ from pointer passed >> to splitted function? How do I know then which pointer is to be passed >> with bounds and wchich one is not? Moreover current ABI does not allow >> to pass bounds with no pointer or pass bounds for some pointers in the >> call only. > > But I don't see any case in function splitting where we're going to want to > pass the pointer without the bounds. If you want the former, you're going > to want the latter. There are at least cases when checks are eliminated or when lots of pointer usages are accompanied with few checks performed earlier (e.g. we are working with array). In such cases splitted part may easily get no bounds. > > I really don't see why you need to do anything special here. At the most an > assert in the splitting code to ensure that you don't have a situation where > there's mixed pointers with bounds and pointers without bounds should be all > you need or that you passed a bounds with no associated pointer :-) It would also require generation of proper bind_bounds calls in the original function and arg_bounds calls in a separated part. So, special support is required. > > > Jeff
On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2013/11/18 Jeff Law <law@redhat.com>: >> On 11/18/13 11:27, Ilya Enkovich wrote: >>> >>> >>> How does pointer passed to regular function differ from pointer passed >>> to splitted function? How do I know then which pointer is to be passed >>> with bounds and wchich one is not? Moreover current ABI does not allow >>> to pass bounds with no pointer or pass bounds for some pointers in the >>> call only. >> >> But I don't see any case in function splitting where we're going to want to >> pass the pointer without the bounds. If you want the former, you're going >> to want the latter. > > There are at least cases when checks are eliminated or when lots of > pointer usages are accompanied with few checks performed earlier (e.g. > we are working with array). In such cases splitted part may easily get > no bounds. > >> >> I really don't see why you need to do anything special here. At the most an >> assert in the splitting code to ensure that you don't have a situation where >> there's mixed pointers with bounds and pointers without bounds should be all >> you need or that you passed a bounds with no associated pointer :-) > > It would also require generation of proper bind_bounds calls in the > original function and arg_bounds calls in a separated part. So, > special support is required. Well, only to keep proper instrumentation. I hope code still works (doesn't trap) when optimizations "wreck" the bounds? Thus all these patches are improving bounds propagation but are not required for correctness? If so please postpone all of them until after the initial support is merged. If not, please make sure BND instrumentation works conservatively when optimizations wreck it. Richard. >> >> >> Jeff
2013/11/19 Richard Biener <richard.guenther@gmail.com>: > On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2013/11/18 Jeff Law <law@redhat.com>: >>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>> >>>> >>>> How does pointer passed to regular function differ from pointer passed >>>> to splitted function? How do I know then which pointer is to be passed >>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>> call only. >>> >>> But I don't see any case in function splitting where we're going to want to >>> pass the pointer without the bounds. If you want the former, you're going >>> to want the latter. >> >> There are at least cases when checks are eliminated or when lots of >> pointer usages are accompanied with few checks performed earlier (e.g. >> we are working with array). In such cases splitted part may easily get >> no bounds. >> >>> >>> I really don't see why you need to do anything special here. At the most an >>> assert in the splitting code to ensure that you don't have a situation where >>> there's mixed pointers with bounds and pointers without bounds should be all >>> you need or that you passed a bounds with no associated pointer :-) >> >> It would also require generation of proper bind_bounds calls in the >> original function and arg_bounds calls in a separated part. So, >> special support is required. > > Well, only to keep proper instrumentation. I hope code still works > (doesn't trap) when optimizations "wreck" the bounds? Thus all > these patches are improving bounds propagation but are not required > for correctness? If so please postpone all of them until after the > initial support is merged. If not, please make sure BND instrumentation > works conservatively when optimizations wreck it. All patches I sent for optimization passes are required to avoid ICEs when compiling instrumented code. Ilya > > Richard. > >>> >>> >>> Jeff
On 11/19/13 05:20, Ilya Enkovich wrote: > 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2013/11/18 Jeff Law <law@redhat.com>: >>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>> >>>>> >>>>> How does pointer passed to regular function differ from pointer passed >>>>> to splitted function? How do I know then which pointer is to be passed >>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>> call only. >>>> >>>> But I don't see any case in function splitting where we're going to want to >>>> pass the pointer without the bounds. If you want the former, you're going >>>> to want the latter. >>> >>> There are at least cases when checks are eliminated or when lots of >>> pointer usages are accompanied with few checks performed earlier (e.g. >>> we are working with array). In such cases splitted part may easily get >>> no bounds. >>> >>>> >>>> I really don't see why you need to do anything special here. At the most an >>>> assert in the splitting code to ensure that you don't have a situation where >>>> there's mixed pointers with bounds and pointers without bounds should be all >>>> you need or that you passed a bounds with no associated pointer :-) >>> >>> It would also require generation of proper bind_bounds calls in the >>> original function and arg_bounds calls in a separated part. So, >>> special support is required. >> >> Well, only to keep proper instrumentation. I hope code still works >> (doesn't trap) when optimizations "wreck" the bounds? Thus all >> these patches are improving bounds propagation but are not required >> for correctness? If so please postpone all of them until after the >> initial support is merged. If not, please make sure BND instrumentation >> works conservatively when optimizations wreck it. > > All patches I sent for optimization passes are required to avoid ICEs > when compiling instrumented code. Then I think we're going to need to understand them in more detail. That's going to mean testcases, probably dumps and some commentary about what went wrong. I can't speak for Richi, but when optimizations get disabled, I tend to want to really understand why and make sure we're not papering over a larger problem. The tail recursion elimination one we're discussing now is a great example. At this point I understand the problem you're running into, but I'm still trying to wrap my head around the implications of the funny semantics of __builtin_arg_bounds and how they may cause other problems. jeff
On 11/18/13 12:12, Ilya Enkovich wrote: > 2013/11/18 Jeff Law <law@redhat.com>: >> On 11/18/13 11:27, Ilya Enkovich wrote: >>> >>> >>> How does pointer passed to regular function differ from pointer passed >>> to splitted function? How do I know then which pointer is to be passed >>> with bounds and wchich one is not? Moreover current ABI does not allow >>> to pass bounds with no pointer or pass bounds for some pointers in the >>> call only. >> >> But I don't see any case in function splitting where we're going to want to >> pass the pointer without the bounds. If you want the former, you're going >> to want the latter. > > There are at least cases when checks are eliminated or when lots of > pointer usages are accompanied with few checks performed earlier (e.g. > we are working with array). In such cases splitted part may easily get > no bounds. So your assertion is we can have a split point that sits between the checks and uses of a bounded pointer? I can buy that, so to speak. I'll also note that the all-or-nothing nature of the ABI is part of the problem here. Jeff
On 11/18/13 12:12, Ilya Enkovich wrote: > 2013/11/18 Jeff Law <law@redhat.com>: >> On 11/18/13 11:27, Ilya Enkovich wrote: >>> >>> >>> How does pointer passed to regular function differ from pointer passed >>> to splitted function? How do I know then which pointer is to be passed >>> with bounds and wchich one is not? Moreover current ABI does not allow >>> to pass bounds with no pointer or pass bounds for some pointers in the >>> call only. >> >> But I don't see any case in function splitting where we're going to want to >> pass the pointer without the bounds. If you want the former, you're going >> to want the latter. > > There are at least cases when checks are eliminated or when lots of > pointer usages are accompanied with few checks performed earlier (e.g. > we are working with array). In such cases splitted part may easily get > no bounds. This patch is fine. You can install it as long as any prerequisites are already installed. Thanks, Jeff
2013/11/19 Jeff Law <law@redhat.com>: > On 11/19/13 05:20, Ilya Enkovich wrote: >> >> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>> >>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>> wrote: >>>> >>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>> >>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>> >>>>>> >>>>>> >>>>>> How does pointer passed to regular function differ from pointer passed >>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>> call only. >>>>> >>>>> >>>>> But I don't see any case in function splitting where we're going to >>>>> want to >>>>> pass the pointer without the bounds. If you want the former, you're >>>>> going >>>>> to want the latter. >>>> >>>> >>>> There are at least cases when checks are eliminated or when lots of >>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>> we are working with array). In such cases splitted part may easily get >>>> no bounds. >>>> >>>>> >>>>> I really don't see why you need to do anything special here. At the >>>>> most an >>>>> assert in the splitting code to ensure that you don't have a situation >>>>> where >>>>> there's mixed pointers with bounds and pointers without bounds should >>>>> be all >>>>> you need or that you passed a bounds with no associated pointer :-) >>>> >>>> >>>> It would also require generation of proper bind_bounds calls in the >>>> original function and arg_bounds calls in a separated part. So, >>>> special support is required. >>> >>> >>> Well, only to keep proper instrumentation. I hope code still works >>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>> these patches are improving bounds propagation but are not required >>> for correctness? If so please postpone all of them until after the >>> initial support is merged. If not, please make sure BND instrumentation >>> works conservatively when optimizations wreck it. >> >> >> All patches I sent for optimization passes are required to avoid ICEs >> when compiling instrumented code. > > Then I think we're going to need to understand them in more detail. That's > going to mean testcases, probably dumps and some commentary about what went > wrong. > > I can't speak for Richi, but when optimizations get disabled, I tend to want > to really understand why and make sure we're not papering over a larger > problem. > > The tail recursion elimination one we're discussing now is a great example. > At this point I understand the problem you're running into, but I'm still > trying to wrap my head around the implications of the funny semantics of > __builtin_arg_bounds and how they may cause other problems. Root of all problems if implicit data flow hidden in arg_bounds and bind_bounds. Calls consume bounds and compiler does not know it. And input bounds are always expressed via arg_bounds calls and never expressed via formal args. Obviously optimizers have to be taught about these data dependencies to work correctly. I agree semantics of arg_bounds call creates many issues for optimizers but currently I do not see a better replacement for it. Ilya > > > jeff >
On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2013/11/19 Jeff Law <law@redhat.com>: >> On 11/19/13 05:20, Ilya Enkovich wrote: >>> >>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>> >>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>> wrote: >>>>> >>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>> >>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>> call only. >>>>>> >>>>>> >>>>>> But I don't see any case in function splitting where we're going to >>>>>> want to >>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>> going >>>>>> to want the latter. >>>>> >>>>> >>>>> There are at least cases when checks are eliminated or when lots of >>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>> we are working with array). In such cases splitted part may easily get >>>>> no bounds. >>>>> >>>>>> >>>>>> I really don't see why you need to do anything special here. At the >>>>>> most an >>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>> where >>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>> be all >>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>> >>>>> >>>>> It would also require generation of proper bind_bounds calls in the >>>>> original function and arg_bounds calls in a separated part. So, >>>>> special support is required. >>>> >>>> >>>> Well, only to keep proper instrumentation. I hope code still works >>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>> these patches are improving bounds propagation but are not required >>>> for correctness? If so please postpone all of them until after the >>>> initial support is merged. If not, please make sure BND instrumentation >>>> works conservatively when optimizations wreck it. >>> >>> >>> All patches I sent for optimization passes are required to avoid ICEs >>> when compiling instrumented code. >> >> Then I think we're going to need to understand them in more detail. That's >> going to mean testcases, probably dumps and some commentary about what went >> wrong. >> >> I can't speak for Richi, but when optimizations get disabled, I tend to want >> to really understand why and make sure we're not papering over a larger >> problem. >> >> The tail recursion elimination one we're discussing now is a great example. >> At this point I understand the problem you're running into, but I'm still >> trying to wrap my head around the implications of the funny semantics of >> __builtin_arg_bounds and how they may cause other problems. > > Root of all problems if implicit data flow hidden in arg_bounds and > bind_bounds. Calls consume bounds and compiler does not know it. And > input bounds are always expressed via arg_bounds calls and never > expressed via formal args. Obviously optimizers have to be taught > about these data dependencies to work correctly. > > I agree semantics of arg_bounds call creates many issues for > optimizers but currently I do not see a better replacement for it. But it looks incredibly fragile if you ICE once something you don't like happens. You should be able to easily detect the case and "punt", that is, drop to non-instrumented aka invalidating bounds. Thus, I really really don't like these patches. They hint at some deeper problem with the overall design (or the HW feature or the accompaning ABI). Richard. > Ilya > >> >> >> jeff >>
On Wed, Nov 20, 2013 at 10:57 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2013/11/19 Jeff Law <law@redhat.com>: >>> On 11/19/13 05:20, Ilya Enkovich wrote: >>>> >>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>>> >>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>>> wrote: >>>>>> >>>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>>> >>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>>> call only. >>>>>>> >>>>>>> >>>>>>> But I don't see any case in function splitting where we're going to >>>>>>> want to >>>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>>> going >>>>>>> to want the latter. >>>>>> >>>>>> >>>>>> There are at least cases when checks are eliminated or when lots of >>>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>>> we are working with array). In such cases splitted part may easily get >>>>>> no bounds. >>>>>> >>>>>>> >>>>>>> I really don't see why you need to do anything special here. At the >>>>>>> most an >>>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>>> where >>>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>>> be all >>>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>>> >>>>>> >>>>>> It would also require generation of proper bind_bounds calls in the >>>>>> original function and arg_bounds calls in a separated part. So, >>>>>> special support is required. >>>>> >>>>> >>>>> Well, only to keep proper instrumentation. I hope code still works >>>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>>> these patches are improving bounds propagation but are not required >>>>> for correctness? If so please postpone all of them until after the >>>>> initial support is merged. If not, please make sure BND instrumentation >>>>> works conservatively when optimizations wreck it. >>>> >>>> >>>> All patches I sent for optimization passes are required to avoid ICEs >>>> when compiling instrumented code. >>> >>> Then I think we're going to need to understand them in more detail. That's >>> going to mean testcases, probably dumps and some commentary about what went >>> wrong. >>> >>> I can't speak for Richi, but when optimizations get disabled, I tend to want >>> to really understand why and make sure we're not papering over a larger >>> problem. >>> >>> The tail recursion elimination one we're discussing now is a great example. >>> At this point I understand the problem you're running into, but I'm still >>> trying to wrap my head around the implications of the funny semantics of >>> __builtin_arg_bounds and how they may cause other problems. >> >> Root of all problems if implicit data flow hidden in arg_bounds and >> bind_bounds. Calls consume bounds and compiler does not know it. And >> input bounds are always expressed via arg_bounds calls and never >> expressed via formal args. Obviously optimizers have to be taught >> about these data dependencies to work correctly. >> >> I agree semantics of arg_bounds call creates many issues for >> optimizers but currently I do not see a better replacement for it. > > But it looks incredibly fragile if you ICE once something you don't like > happens. You should be able to easily detect the case and "punt", > that is, drop to non-instrumented aka invalidating bounds. > > Thus, I really really don't like these patches. They hint at some > deeper problem with the overall design (or the HW feature or the > accompaning ABI). Note that this, the intrusiveness of the feature and the questionable gain makes me question whether GCC should have support for this feature (and whether we really should rush this in this late). Thus, I hereby formally ask to push back this feature to 4.10. Thanks, Richard. > Richard. > >> Ilya >> >>> >>> >>> jeff >>>
2013/11/20 Richard Biener <richard.guenther@gmail.com>: > On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2013/11/19 Jeff Law <law@redhat.com>: >>> On 11/19/13 05:20, Ilya Enkovich wrote: >>>> >>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>>> >>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>>> wrote: >>>>>> >>>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>>> >>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>>> call only. >>>>>>> >>>>>>> >>>>>>> But I don't see any case in function splitting where we're going to >>>>>>> want to >>>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>>> going >>>>>>> to want the latter. >>>>>> >>>>>> >>>>>> There are at least cases when checks are eliminated or when lots of >>>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>>> we are working with array). In such cases splitted part may easily get >>>>>> no bounds. >>>>>> >>>>>>> >>>>>>> I really don't see why you need to do anything special here. At the >>>>>>> most an >>>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>>> where >>>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>>> be all >>>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>>> >>>>>> >>>>>> It would also require generation of proper bind_bounds calls in the >>>>>> original function and arg_bounds calls in a separated part. So, >>>>>> special support is required. >>>>> >>>>> >>>>> Well, only to keep proper instrumentation. I hope code still works >>>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>>> these patches are improving bounds propagation but are not required >>>>> for correctness? If so please postpone all of them until after the >>>>> initial support is merged. If not, please make sure BND instrumentation >>>>> works conservatively when optimizations wreck it. >>>> >>>> >>>> All patches I sent for optimization passes are required to avoid ICEs >>>> when compiling instrumented code. >>> >>> Then I think we're going to need to understand them in more detail. That's >>> going to mean testcases, probably dumps and some commentary about what went >>> wrong. >>> >>> I can't speak for Richi, but when optimizations get disabled, I tend to want >>> to really understand why and make sure we're not papering over a larger >>> problem. >>> >>> The tail recursion elimination one we're discussing now is a great example. >>> At this point I understand the problem you're running into, but I'm still >>> trying to wrap my head around the implications of the funny semantics of >>> __builtin_arg_bounds and how they may cause other problems. >> >> Root of all problems if implicit data flow hidden in arg_bounds and >> bind_bounds. Calls consume bounds and compiler does not know it. And >> input bounds are always expressed via arg_bounds calls and never >> expressed via formal args. Obviously optimizers have to be taught >> about these data dependencies to work correctly. >> >> I agree semantics of arg_bounds call creates many issues for >> optimizers but currently I do not see a better replacement for it. > > But it looks incredibly fragile if you ICE once something you don't like > happens. You should be able to easily detect the case and "punt", > that is, drop to non-instrumented aka invalidating bounds. Actually, it is easy detect such cases and invalidate bounds each time some optimization bounds data flow. With such feature 5-6 of sent patches would be unnecessary to successfully build instrumented code on -O2, but without them quality of checks would be much lower (mostly due to inline). Ilya > > Thus, I really really don't like these patches. They hint at some > deeper problem with the overall design (or the HW feature or the > accompaning ABI). > > Richard. > >> Ilya >> >>> >>> >>> jeff >>>
On Wed, Nov 20, 2013 at 11:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2013/11/20 Richard Biener <richard.guenther@gmail.com>: >> On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2013/11/19 Jeff Law <law@redhat.com>: >>>> On 11/19/13 05:20, Ilya Enkovich wrote: >>>>> >>>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>>>> >>>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>>>> >>>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>>>> call only. >>>>>>>> >>>>>>>> >>>>>>>> But I don't see any case in function splitting where we're going to >>>>>>>> want to >>>>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>>>> going >>>>>>>> to want the latter. >>>>>>> >>>>>>> >>>>>>> There are at least cases when checks are eliminated or when lots of >>>>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>>>> we are working with array). In such cases splitted part may easily get >>>>>>> no bounds. >>>>>>> >>>>>>>> >>>>>>>> I really don't see why you need to do anything special here. At the >>>>>>>> most an >>>>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>>>> where >>>>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>>>> be all >>>>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>>>> >>>>>>> >>>>>>> It would also require generation of proper bind_bounds calls in the >>>>>>> original function and arg_bounds calls in a separated part. So, >>>>>>> special support is required. >>>>>> >>>>>> >>>>>> Well, only to keep proper instrumentation. I hope code still works >>>>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>>>> these patches are improving bounds propagation but are not required >>>>>> for correctness? If so please postpone all of them until after the >>>>>> initial support is merged. If not, please make sure BND instrumentation >>>>>> works conservatively when optimizations wreck it. >>>>> >>>>> >>>>> All patches I sent for optimization passes are required to avoid ICEs >>>>> when compiling instrumented code. >>>> >>>> Then I think we're going to need to understand them in more detail. That's >>>> going to mean testcases, probably dumps and some commentary about what went >>>> wrong. >>>> >>>> I can't speak for Richi, but when optimizations get disabled, I tend to want >>>> to really understand why and make sure we're not papering over a larger >>>> problem. >>>> >>>> The tail recursion elimination one we're discussing now is a great example. >>>> At this point I understand the problem you're running into, but I'm still >>>> trying to wrap my head around the implications of the funny semantics of >>>> __builtin_arg_bounds and how they may cause other problems. >>> >>> Root of all problems if implicit data flow hidden in arg_bounds and >>> bind_bounds. Calls consume bounds and compiler does not know it. And >>> input bounds are always expressed via arg_bounds calls and never >>> expressed via formal args. Obviously optimizers have to be taught >>> about these data dependencies to work correctly. >>> >>> I agree semantics of arg_bounds call creates many issues for >>> optimizers but currently I do not see a better replacement for it. >> >> But it looks incredibly fragile if you ICE once something you don't like >> happens. You should be able to easily detect the case and "punt", >> that is, drop to non-instrumented aka invalidating bounds. > > Actually, it is easy detect such cases and invalidate bounds each time > some optimization bounds data flow. With such feature 5-6 of sent > patches would be unnecessary to successfully build instrumented code > on -O2, but without them quality of checks would be much lower (mostly > due to inline). Sure, but you want to have this feature for a production compiler as for sure you are going to miss an odd sequence of transforms that breaks your assumptions. Richard. > Ilya > >> >> Thus, I really really don't like these patches. They hint at some >> deeper problem with the overall design (or the HW feature or the >> accompaning ABI). >> >> Richard. >> >>> Ilya >>> >>>> >>>> >>>> jeff >>>>
2013/11/20 Richard Biener <richard.guenther@gmail.com>: > On Wed, Nov 20, 2013 at 11:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2013/11/20 Richard Biener <richard.guenther@gmail.com>: >>> On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2013/11/19 Jeff Law <law@redhat.com>: >>>>> On 11/19/13 05:20, Ilya Enkovich wrote: >>>>>> >>>>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>>>>> >>>>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>>>>> >>>>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>>>>> call only. >>>>>>>>> >>>>>>>>> >>>>>>>>> But I don't see any case in function splitting where we're going to >>>>>>>>> want to >>>>>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>>>>> going >>>>>>>>> to want the latter. >>>>>>>> >>>>>>>> >>>>>>>> There are at least cases when checks are eliminated or when lots of >>>>>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>>>>> we are working with array). In such cases splitted part may easily get >>>>>>>> no bounds. >>>>>>>> >>>>>>>>> >>>>>>>>> I really don't see why you need to do anything special here. At the >>>>>>>>> most an >>>>>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>>>>> where >>>>>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>>>>> be all >>>>>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>>>>> >>>>>>>> >>>>>>>> It would also require generation of proper bind_bounds calls in the >>>>>>>> original function and arg_bounds calls in a separated part. So, >>>>>>>> special support is required. >>>>>>> >>>>>>> >>>>>>> Well, only to keep proper instrumentation. I hope code still works >>>>>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>>>>> these patches are improving bounds propagation but are not required >>>>>>> for correctness? If so please postpone all of them until after the >>>>>>> initial support is merged. If not, please make sure BND instrumentation >>>>>>> works conservatively when optimizations wreck it. >>>>>> >>>>>> >>>>>> All patches I sent for optimization passes are required to avoid ICEs >>>>>> when compiling instrumented code. >>>>> >>>>> Then I think we're going to need to understand them in more detail. That's >>>>> going to mean testcases, probably dumps and some commentary about what went >>>>> wrong. >>>>> >>>>> I can't speak for Richi, but when optimizations get disabled, I tend to want >>>>> to really understand why and make sure we're not papering over a larger >>>>> problem. >>>>> >>>>> The tail recursion elimination one we're discussing now is a great example. >>>>> At this point I understand the problem you're running into, but I'm still >>>>> trying to wrap my head around the implications of the funny semantics of >>>>> __builtin_arg_bounds and how they may cause other problems. >>>> >>>> Root of all problems if implicit data flow hidden in arg_bounds and >>>> bind_bounds. Calls consume bounds and compiler does not know it. And >>>> input bounds are always expressed via arg_bounds calls and never >>>> expressed via formal args. Obviously optimizers have to be taught >>>> about these data dependencies to work correctly. >>>> >>>> I agree semantics of arg_bounds call creates many issues for >>>> optimizers but currently I do not see a better replacement for it. >>> >>> But it looks incredibly fragile if you ICE once something you don't like >>> happens. You should be able to easily detect the case and "punt", >>> that is, drop to non-instrumented aka invalidating bounds. >> >> Actually, it is easy detect such cases and invalidate bounds each time >> some optimization bounds data flow. With such feature 5-6 of sent >> patches would be unnecessary to successfully build instrumented code >> on -O2, but without them quality of checks would be much lower (mostly >> due to inline). > > Sure, but you want to have this feature for a production compiler as > for sure you are going to miss an odd sequence of transforms that > breaks your assumptions. There still should be a way to detect such cases. If all unsupported cases are silently ignored then user never knows about it and never reports the problem. I may fix expand changes to make it less vulnerable to different code modifications and get no ICEs without fixes in optimizers (well, except fix in tree-ssa-ccp.c where instrumentation breaks assumption for data flow between stack save/restore builtins). But even with stable expand I would still want to have all those changes in optimizers to get better checker quality. Ilya > > Richard. > >> Ilya >> >>> >>> Thus, I really really don't like these patches. They hint at some >>> deeper problem with the overall design (or the HW feature or the >>> accompaning ABI). >>> >>> Richard. >>> >>>> Ilya >>>> >>>>> >>>>> >>>>> jeff >>>>>
On 11/20/13 03:02, Richard Biener wrote: > > Note that this, the intrusiveness of the feature and the questionable > gain makes me question whether GCC should have support for this > feature (and whether we really should rush this in this late). > > Thus, I hereby formally ask to push back this feature to 4.10. Sigh. I'd hoped we were making progress and Ilya could have things wrapped up in a reasonable amount of time. But I certainly see your point of view and I have some concerns about the semantics of the builtins now that we're getting deeper into the bits. The patches were posted long ago (back in mid Sept) and received little/no feedback at that time. Ilya played by the rules and it was our failing as maintainers that caused things to back up. Thus I believe the code should be given fair consideration for inclusion into 4.9. -- I suspect the hardware implementation and ABI are largely set by the need to interoperate with uninstrumented code. Where I think the patchset falls down is in implementation details. Anyway, if you're going to stick with your formal request to postpone until after 4.9, I'm not going to push hard from the other direction. Given that, we should probably pull out the half-dozen preparatory patches that went in. jeff
On 11/20/13 03:55, Ilya Enkovich wrote: >> But it looks incredibly fragile if you ICE once something you don't like >> happens. You should be able to easily detect the case and "punt", >> that is, drop to non-instrumented aka invalidating bounds. > > Actually, it is easy detect such cases and invalidate bounds each time > some optimization bounds data flow. With such feature 5-6 of sent > patches would be unnecessary to successfully build instrumented code > on -O2, but without them quality of checks would be much lower (mostly > due to inline). It seems to me the right thing to do is warn in those cases where you have to invalidate the bounds. That's going to be less controversial at this stage than something like disabling tail recursion elimination. Those warning sites also form a todo list of things to work on after the code is integrated and functioning at a basic level. And when the time comes to work on those problems, we have a reference implementation & testcase that is dropping to uninstrumented. For myself at least, that's much easier to deal with because I can throw the testcase into a debugging session and poke at it. I pick up stuff much faster that way. Jeff
On 11/19/13 13:18, Ilya Enkovich wrote: > > Root of all problems if implicit data flow hidden in arg_bounds and > bind_bounds. Calls consume bounds and compiler does not know it. And > input bounds are always expressed via arg_bounds calls and never > expressed via formal args. Obviously optimizers have to be taught > about these data dependencies to work correctly. > > I agree semantics of arg_bounds call creates many issues for > optimizers but currently I do not see a better replacement for it. Well, there's hidden dataflow, but there's also "magic" that has to happen in the prologue and function calling sequences. The "magic" has rules that it expects the rest of the compiler to play by, but nobody other than yourself probably understands what those rules are and what passes might violate those rules. Jeff
2013/11/20 Richard Biener <richard.guenther@gmail.com>: > On Wed, Nov 20, 2013 at 10:57 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2013/11/19 Jeff Law <law@redhat.com>: >>>> On 11/19/13 05:20, Ilya Enkovich wrote: >>>>> >>>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>>>> >>>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>>>> >>>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>>>> call only. >>>>>>>> >>>>>>>> >>>>>>>> But I don't see any case in function splitting where we're going to >>>>>>>> want to >>>>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>>>> going >>>>>>>> to want the latter. >>>>>>> >>>>>>> >>>>>>> There are at least cases when checks are eliminated or when lots of >>>>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>>>> we are working with array). In such cases splitted part may easily get >>>>>>> no bounds. >>>>>>> >>>>>>>> >>>>>>>> I really don't see why you need to do anything special here. At the >>>>>>>> most an >>>>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>>>> where >>>>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>>>> be all >>>>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>>>> >>>>>>> >>>>>>> It would also require generation of proper bind_bounds calls in the >>>>>>> original function and arg_bounds calls in a separated part. So, >>>>>>> special support is required. >>>>>> >>>>>> >>>>>> Well, only to keep proper instrumentation. I hope code still works >>>>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>>>> these patches are improving bounds propagation but are not required >>>>>> for correctness? If so please postpone all of them until after the >>>>>> initial support is merged. If not, please make sure BND instrumentation >>>>>> works conservatively when optimizations wreck it. >>>>> >>>>> >>>>> All patches I sent for optimization passes are required to avoid ICEs >>>>> when compiling instrumented code. >>>> >>>> Then I think we're going to need to understand them in more detail. That's >>>> going to mean testcases, probably dumps and some commentary about what went >>>> wrong. >>>> >>>> I can't speak for Richi, but when optimizations get disabled, I tend to want >>>> to really understand why and make sure we're not papering over a larger >>>> problem. >>>> >>>> The tail recursion elimination one we're discussing now is a great example. >>>> At this point I understand the problem you're running into, but I'm still >>>> trying to wrap my head around the implications of the funny semantics of >>>> __builtin_arg_bounds and how they may cause other problems. >>> >>> Root of all problems if implicit data flow hidden in arg_bounds and >>> bind_bounds. Calls consume bounds and compiler does not know it. And >>> input bounds are always expressed via arg_bounds calls and never >>> expressed via formal args. Obviously optimizers have to be taught >>> about these data dependencies to work correctly. >>> >>> I agree semantics of arg_bounds call creates many issues for >>> optimizers but currently I do not see a better replacement for it. >> >> But it looks incredibly fragile if you ICE once something you don't like >> happens. You should be able to easily detect the case and "punt", >> that is, drop to non-instrumented aka invalidating bounds. >> >> Thus, I really really don't like these patches. They hint at some >> deeper problem with the overall design (or the HW feature or the >> accompaning ABI). > > Note that this, the intrusiveness of the feature and the questionable > gain makes me question whether GCC should have support for this > feature (and whether we really should rush this in this late). > > Thus, I hereby formally ask to push back this feature to 4.10. I think you overestimate the intrusiveness of the checker. Necessity of changes in optimization passes is artificial and is used to get maximum checking quality. It can be easily made immune for different code transformation by simple changes in the process of instrumentation expand (I have a fix for that already). With that change only pass itself, support for bound args during expand, support in i386 target and minor infrastructure changes are required (e.g. flag in varpool_node, bounds_constants). Changes in inline, propagation, SRA, tail recursion, strlen, function splitting, string function builtins expand would become optional and affect checking quality only. Also note that all changes do not affect compilation process when no instrumentation is used. Please reconsider your decision about pushing it to 4.10 taking that into account. Thanks, Ilya > > Thanks, > Richard. > >> Richard. >> >>> Ilya >>> >>>> >>>> >>>> jeff >>>>
On Thu, Nov 21, 2013 at 10:31 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2013/11/20 Richard Biener <richard.guenther@gmail.com>: >> On Wed, Nov 20, 2013 at 10:57 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Tue, Nov 19, 2013 at 9:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2013/11/19 Jeff Law <law@redhat.com>: >>>>> On 11/19/13 05:20, Ilya Enkovich wrote: >>>>>> >>>>>> 2013/11/19 Richard Biener <richard.guenther@gmail.com>: >>>>>>> >>>>>>> On Mon, Nov 18, 2013 at 8:12 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> 2013/11/18 Jeff Law <law@redhat.com>: >>>>>>>>> >>>>>>>>> On 11/18/13 11:27, Ilya Enkovich wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> How does pointer passed to regular function differ from pointer passed >>>>>>>>>> to splitted function? How do I know then which pointer is to be passed >>>>>>>>>> with bounds and wchich one is not? Moreover current ABI does not allow >>>>>>>>>> to pass bounds with no pointer or pass bounds for some pointers in the >>>>>>>>>> call only. >>>>>>>>> >>>>>>>>> >>>>>>>>> But I don't see any case in function splitting where we're going to >>>>>>>>> want to >>>>>>>>> pass the pointer without the bounds. If you want the former, you're >>>>>>>>> going >>>>>>>>> to want the latter. >>>>>>>> >>>>>>>> >>>>>>>> There are at least cases when checks are eliminated or when lots of >>>>>>>> pointer usages are accompanied with few checks performed earlier (e.g. >>>>>>>> we are working with array). In such cases splitted part may easily get >>>>>>>> no bounds. >>>>>>>> >>>>>>>>> >>>>>>>>> I really don't see why you need to do anything special here. At the >>>>>>>>> most an >>>>>>>>> assert in the splitting code to ensure that you don't have a situation >>>>>>>>> where >>>>>>>>> there's mixed pointers with bounds and pointers without bounds should >>>>>>>>> be all >>>>>>>>> you need or that you passed a bounds with no associated pointer :-) >>>>>>>> >>>>>>>> >>>>>>>> It would also require generation of proper bind_bounds calls in the >>>>>>>> original function and arg_bounds calls in a separated part. So, >>>>>>>> special support is required. >>>>>>> >>>>>>> >>>>>>> Well, only to keep proper instrumentation. I hope code still works >>>>>>> (doesn't trap) when optimizations "wreck" the bounds? Thus all >>>>>>> these patches are improving bounds propagation but are not required >>>>>>> for correctness? If so please postpone all of them until after the >>>>>>> initial support is merged. If not, please make sure BND instrumentation >>>>>>> works conservatively when optimizations wreck it. >>>>>> >>>>>> >>>>>> All patches I sent for optimization passes are required to avoid ICEs >>>>>> when compiling instrumented code. >>>>> >>>>> Then I think we're going to need to understand them in more detail. That's >>>>> going to mean testcases, probably dumps and some commentary about what went >>>>> wrong. >>>>> >>>>> I can't speak for Richi, but when optimizations get disabled, I tend to want >>>>> to really understand why and make sure we're not papering over a larger >>>>> problem. >>>>> >>>>> The tail recursion elimination one we're discussing now is a great example. >>>>> At this point I understand the problem you're running into, but I'm still >>>>> trying to wrap my head around the implications of the funny semantics of >>>>> __builtin_arg_bounds and how they may cause other problems. >>>> >>>> Root of all problems if implicit data flow hidden in arg_bounds and >>>> bind_bounds. Calls consume bounds and compiler does not know it. And >>>> input bounds are always expressed via arg_bounds calls and never >>>> expressed via formal args. Obviously optimizers have to be taught >>>> about these data dependencies to work correctly. >>>> >>>> I agree semantics of arg_bounds call creates many issues for >>>> optimizers but currently I do not see a better replacement for it. >>> >>> But it looks incredibly fragile if you ICE once something you don't like >>> happens. You should be able to easily detect the case and "punt", >>> that is, drop to non-instrumented aka invalidating bounds. >>> >>> Thus, I really really don't like these patches. They hint at some >>> deeper problem with the overall design (or the HW feature or the >>> accompaning ABI). >> >> Note that this, the intrusiveness of the feature and the questionable >> gain makes me question whether GCC should have support for this >> feature (and whether we really should rush this in this late). >> >> Thus, I hereby formally ask to push back this feature to 4.10. > > I think you overestimate the intrusiveness of the checker. Necessity > of changes in optimization passes is artificial and is used to get > maximum checking quality. It can be easily made immune for different > code transformation by simple changes in the process of > instrumentation expand (I have a fix for that already). With that > change only pass itself, support for bound args during expand, support > in i386 target and minor infrastructure changes are required (e.g. > flag in varpool_node, bounds_constants). Changes in inline, > propagation, SRA, tail recursion, strlen, function splitting, string > function builtins expand would become optional and affect checking > quality only. > > Also note that all changes do not affect compilation process when no > instrumentation is used. > > Please reconsider your decision about pushing it to 4.10 taking that > into account. The point is that we are still pondering over the design and stage1 is basically over. Richard. > Thanks, > Ilya > >> >> Thanks, >> Richard. >> >>> Richard. >>> >>>> Ilya >>>> >>>>> >>>>> >>>>> jeff >>>>>
On Wed, Nov 20, 2013 at 7:54 PM, Jeff Law <law@redhat.com> wrote: > On 11/20/13 03:02, Richard Biener wrote: >> >> >> Note that this, the intrusiveness of the feature and the questionable >> gain makes me question whether GCC should have support for this >> feature (and whether we really should rush this in this late). >> >> Thus, I hereby formally ask to push back this feature to 4.10. > > Sigh. I'd hoped we were making progress and Ilya could have things wrapped > up in a reasonable amount of time. But I certainly see your point of view > and I have some concerns about the semantics of the builtins now that we're > getting deeper into the bits. > > > The patches were posted long ago (back in mid Sept) and received little/no > feedback at that time. Ilya played by the rules and it was our failing as > maintainers that caused things to back up. Thus I believe the code should > be given fair consideration for inclusion into 4.9. Note that we wouldn't get anywhere near a release if we apply this "rule". That maintainers time is not infinite is unfortunate but a fact :/ > -- > > I suspect the hardware implementation and ABI are largely set by the need to > interoperate with uninstrumented code. Where I think the patchset falls > down is in implementation details. > > Anyway, if you're going to stick with your formal request to postpone until > after 4.9, I'm not going to push hard from the other direction. Given that, > we should probably pull out the half-dozen preparatory patches that went in. For the latter I was confused by partly applying a series for a feature that hasn't been fully reviewed anyway. This shouldn't be how merging in a new feature works - you split up the feature into multiple patches to ease review, not to commit it piecewise over some weeks. Richard. > > jeff
2013/11/21 Richard Biener <richard.guenther@gmail.com>: > On Wed, Nov 20, 2013 at 7:54 PM, Jeff Law <law@redhat.com> wrote: >> On 11/20/13 03:02, Richard Biener wrote: >>> >>> >>> Note that this, the intrusiveness of the feature and the questionable >>> gain makes me question whether GCC should have support for this >>> feature (and whether we really should rush this in this late). >>> >>> Thus, I hereby formally ask to push back this feature to 4.10. >> >> Sigh. I'd hoped we were making progress and Ilya could have things wrapped >> up in a reasonable amount of time. But I certainly see your point of view >> and I have some concerns about the semantics of the builtins now that we're >> getting deeper into the bits. >> >> >> The patches were posted long ago (back in mid Sept) and received little/no >> feedback at that time. Ilya played by the rules and it was our failing as >> maintainers that caused things to back up. Thus I believe the code should >> be given fair consideration for inclusion into 4.9. > > Note that we wouldn't get anywhere near a release if we apply this "rule". > That maintainers time is not infinite is unfortunate but a fact :/ > >> -- >> >> I suspect the hardware implementation and ABI are largely set by the need to >> interoperate with uninstrumented code. Where I think the patchset falls >> down is in implementation details. >> >> Anyway, if you're going to stick with your formal request to postpone until >> after 4.9, I'm not going to push hard from the other direction. Given that, >> we should probably pull out the half-dozen preparatory patches that went in. > > For the latter I was confused by partly applying a series for a feature that > hasn't been fully reviewed anyway. This shouldn't be how merging in a > new feature works - you split up the feature into multiple patches to ease > review, not to commit it piecewise over some weeks. I'll prepare a patch to remove committed patches. But the first part of series added new ISA extension support. It is independent from the checker. Should it be OK to keep ISA in trunk? Ilya > > Richard. > >> >> jeff
On 11/25/13 04:12, Ilya Enkovich wrote: > > I'll prepare a patch to remove committed patches. But the first part > of series added new ISA extension support. It is independent from the > checker. Should it be OK to keep ISA in trunk? I think this can/should reasonably be Uros's call. I'm sorry we didn't get this moved far enough to make it into GCC 4.9; if I had thought we were going run into these issues I wouldn't have suggested you check in the preparatory patches only to have to back them out later. Jeff
2013/11/25 Jeff Law <law@redhat.com>: > On 11/25/13 04:12, Ilya Enkovich wrote: >> >> >> I'll prepare a patch to remove committed patches. But the first part >> of series added new ISA extension support. It is independent from the >> checker. Should it be OK to keep ISA in trunk? > > I think this can/should reasonably be Uros's call. > > I'm sorry we didn't get this moved far enough to make it into GCC 4.9; if I > had thought we were going run into these issues I wouldn't have suggested > you check in the preparatory patches only to have to back them out later. I also though we could make it go into 4.9. And taking into account encountered problem I think a short time-out to reconsider some design elements would be useful. My next steps in bounds checker improvement include better support for optimization passes and generic target support, which would not require Intel MPX on hardware to use it. One of the way is to keep going with the current model. It has some problems but proved to work. The main problems are: - Hidden data flow to pass bounds into functions. We have implicit data flow for calls from bind_bounds call in caller and arg_bounds call in callee. Optimization passes do not know about this flow and thus may easily corrupt it (especially IPA passes). - Wrapped values. All call arguments are wrapped by bind_bounds calls. It prevents from some optimizations. E.g. we cannot propagate null pointer value into callee if call is instrumented. Positive points here: - we do not affect basic infrastructure significantly - initial checker implementation does not require enabling optimization passes - it is possible to enable optimization passes for better work with instrumented code; it may be done incrementally - it is implemented and works including some optimizations enabling The other way I see is to try to make all current optimizations work correctly with bounds by making changes in the way we instrument the code. If the main problem is usage of bind_bounds and arg_bounds, then we may try to avoid their usage. The obvious way to do it is to use regular arg passing mechanism using call args and params in function decls. The basic idea is to not modify function during instrumentation, but create a new one. We may either keep the original version of the function (e.g. for inline into non-instrumented callers) or just leave it's declaration to be used for non-instrumented calls. Thus, all functions may result in two nodes in cgraph. One for non-instrumented version and one for instrumented version. Function declaration of the instrumented function would have all bounds in it's param list and instrumented call would have all bounds in it's args list accordingly. Advantage of this approach: - It would decrease number of cases when bounds flow is damaged - It would allow to avoid (or at least decrease) modifications in IPA passes to handle bounds correctly - It would allow to unbind bounds from pointers and perform IPA optimizations for bound values (e.g. if we do not know pointer value but know pointer is unchecked, we may propagate it's bounds value into callee using existing propagation optimization) Surely some doubts also exist: - We have two declarations for actually the same function - Most probably need some changes in PARAM_DECLs to bind bounds params with pointer params - If we Most probably need some modifications in passes which generate new function decls (e.g. propagation or splitting) to generate decl with correctly declared bounds > > Jeff >
2013/11/26 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2013/11/25 Jeff Law <law@redhat.com>: >> On 11/25/13 04:12, Ilya Enkovich wrote: >>> >>> >>> I'll prepare a patch to remove committed patches. But the first part >>> of series added new ISA extension support. It is independent from the >>> checker. Should it be OK to keep ISA in trunk? >> >> I think this can/should reasonably be Uros's call. >> >> I'm sorry we didn't get this moved far enough to make it into GCC 4.9; if I >> had thought we were going run into these issues I wouldn't have suggested >> you check in the preparatory patches only to have to back them out later. > > I also though we could make it go into 4.9. And taking into account > encountered problem I think a short time-out to reconsider some design > elements would be useful. > > My next steps in bounds checker improvement include better support for > optimization passes and generic target support, which would not > require Intel MPX on hardware to use it. One of the way is to keep > going with the current model. It has some problems but proved to > work. The main problems are: > - Hidden data flow to pass bounds into functions. We have implicit > data flow for calls from bind_bounds call in caller and arg_bounds > call in callee. Optimization passes do not know about this flow and > thus may easily corrupt it (especially IPA passes). > - Wrapped values. All call arguments are wrapped by bind_bounds > calls. It prevents from some optimizations. E.g. we cannot propagate > null pointer value into callee if call is instrumented. > > Positive points here: > - we do not affect basic infrastructure significantly > - initial checker implementation does not require enabling optimization passes > - it is possible to enable optimization passes for better work with > instrumented code; it may be done incrementally > - it is implemented and works including some optimizations enabling > > The other way I see is to try to make all current optimizations work > correctly with bounds by making changes in the way we instrument the > code. If the main problem is usage of bind_bounds and arg_bounds, > then we may try to avoid their usage. The obvious way to do it is to > use regular arg passing mechanism using call args and params in > function decls. The basic idea is to not modify function during > instrumentation, but create a new one. We may either keep the > original version of the function (e.g. for inline into > non-instrumented callers) or just leave it's declaration to be used > for non-instrumented calls. > > Thus, all functions may result in two nodes in cgraph. One for > non-instrumented version and one for instrumented version. Function > declaration of the instrumented function would have all bounds in it's > param list and instrumented call would have all bounds in it's args > list accordingly. > > Advantage of this approach: > - It would decrease number of cases when bounds flow is damaged > - It would allow to avoid (or at least decrease) modifications in > IPA passes to handle bounds correctly > - It would allow to unbind bounds from pointers and perform IPA > optimizations for bound values (e.g. if we do not know pointer value > but know pointer is unchecked, we may propagate it's bounds value into > callee using existing propagation optimization) > > Surely some doubts also exist: > - We have two declarations for actually the same function > - Most probably need some changes in PARAM_DECLs to bind bounds > params with pointer params > - If we Most probably need some modifications in passes which > generate new function decls (e.g. propagation or splitting) to > generate decl with correctly declared bounds Accidentally sent the letter before finished it :/ But seems I wrote the most of I wanted to. I know this scheme description with double function declarations is very basic but I would like to get your opinion before diving deeper. Probably current scheme is not so bad and we should leave basic things as it is? Thanks, Ilya > >> >> Jeff >>
On Tue, Nov 26, 2013 at 12:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2013/11/26 Ilya Enkovich <enkovich.gnu@gmail.com>: >> 2013/11/25 Jeff Law <law@redhat.com>: >>> On 11/25/13 04:12, Ilya Enkovich wrote: >>>> >>>> >>>> I'll prepare a patch to remove committed patches. But the first part >>>> of series added new ISA extension support. It is independent from the >>>> checker. Should it be OK to keep ISA in trunk? >>> >>> I think this can/should reasonably be Uros's call. >>> >>> I'm sorry we didn't get this moved far enough to make it into GCC 4.9; if I >>> had thought we were going run into these issues I wouldn't have suggested >>> you check in the preparatory patches only to have to back them out later. >> >> I also though we could make it go into 4.9. And taking into account >> encountered problem I think a short time-out to reconsider some design >> elements would be useful. >> >> My next steps in bounds checker improvement include better support for >> optimization passes and generic target support, which would not >> require Intel MPX on hardware to use it. One of the way is to keep >> going with the current model. It has some problems but proved to >> work. The main problems are: >> - Hidden data flow to pass bounds into functions. We have implicit >> data flow for calls from bind_bounds call in caller and arg_bounds >> call in callee. Optimization passes do not know about this flow and >> thus may easily corrupt it (especially IPA passes). >> - Wrapped values. All call arguments are wrapped by bind_bounds >> calls. It prevents from some optimizations. E.g. we cannot propagate >> null pointer value into callee if call is instrumented. >> >> Positive points here: >> - we do not affect basic infrastructure significantly >> - initial checker implementation does not require enabling optimization passes >> - it is possible to enable optimization passes for better work with >> instrumented code; it may be done incrementally >> - it is implemented and works including some optimizations enabling >> >> The other way I see is to try to make all current optimizations work >> correctly with bounds by making changes in the way we instrument the >> code. If the main problem is usage of bind_bounds and arg_bounds, >> then we may try to avoid their usage. The obvious way to do it is to >> use regular arg passing mechanism using call args and params in >> function decls. The basic idea is to not modify function during >> instrumentation, but create a new one. We may either keep the >> original version of the function (e.g. for inline into >> non-instrumented callers) or just leave it's declaration to be used >> for non-instrumented calls. >> >> Thus, all functions may result in two nodes in cgraph. One for >> non-instrumented version and one for instrumented version. Function >> declaration of the instrumented function would have all bounds in it's >> param list and instrumented call would have all bounds in it's args >> list accordingly. >> >> Advantage of this approach: >> - It would decrease number of cases when bounds flow is damaged >> - It would allow to avoid (or at least decrease) modifications in >> IPA passes to handle bounds correctly >> - It would allow to unbind bounds from pointers and perform IPA >> optimizations for bound values (e.g. if we do not know pointer value >> but know pointer is unchecked, we may propagate it's bounds value into >> callee using existing propagation optimization) >> >> Surely some doubts also exist: >> - We have two declarations for actually the same function >> - Most probably need some changes in PARAM_DECLs to bind bounds >> params with pointer params >> - If we Most probably need some modifications in passes which >> generate new function decls (e.g. propagation or splitting) to >> generate decl with correctly declared bounds > > Accidentally sent the letter before finished it :/ > > But seems I wrote the most of I wanted to. I know this scheme > description with double function declarations is very basic but I > would like to get your opinion before diving deeper. Probably current > scheme is not so bad and we should leave basic things as it is? The idea of making instrumented functions clones of the original function sounds interesting. Can you give some numbers (out of the current implementation) on how much code ends up being instrumented? Like when compiling GCC itself for example? I can see there may be both instrumented and not instrumented calls to a function and thus having two versions of the callee might improve optimization of the non-instrumented call via IPA optimizations that you otherwise have to disable? Generally making dataflow explicit is always prefered. How does making it explicit on the GIMPLE / SSA level affect RTL? (I have not looked at all into how you deal with bounds on the RTL level currently) What model does ICC follow - the dataflow behind the backs or the callgraph + clones? (well, more "closely" I should ask - it probably does a third thing ...) Thanks, Richard. > Thanks, > Ilya > >> >>> >>> Jeff >>>
2013/11/26 Richard Biener <richard.guenther@gmail.com>: > On Tue, Nov 26, 2013 at 12:18 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2013/11/26 Ilya Enkovich <enkovich.gnu@gmail.com>: >>> 2013/11/25 Jeff Law <law@redhat.com>: >>>> On 11/25/13 04:12, Ilya Enkovich wrote: >>>>> >>>>> >>>>> I'll prepare a patch to remove committed patches. But the first part >>>>> of series added new ISA extension support. It is independent from the >>>>> checker. Should it be OK to keep ISA in trunk? >>>> >>>> I think this can/should reasonably be Uros's call. >>>> >>>> I'm sorry we didn't get this moved far enough to make it into GCC 4.9; if I >>>> had thought we were going run into these issues I wouldn't have suggested >>>> you check in the preparatory patches only to have to back them out later. >>> >>> I also though we could make it go into 4.9. And taking into account >>> encountered problem I think a short time-out to reconsider some design >>> elements would be useful. >>> >>> My next steps in bounds checker improvement include better support for >>> optimization passes and generic target support, which would not >>> require Intel MPX on hardware to use it. One of the way is to keep >>> going with the current model. It has some problems but proved to >>> work. The main problems are: >>> - Hidden data flow to pass bounds into functions. We have implicit >>> data flow for calls from bind_bounds call in caller and arg_bounds >>> call in callee. Optimization passes do not know about this flow and >>> thus may easily corrupt it (especially IPA passes). >>> - Wrapped values. All call arguments are wrapped by bind_bounds >>> calls. It prevents from some optimizations. E.g. we cannot propagate >>> null pointer value into callee if call is instrumented. >>> >>> Positive points here: >>> - we do not affect basic infrastructure significantly >>> - initial checker implementation does not require enabling optimization passes >>> - it is possible to enable optimization passes for better work with >>> instrumented code; it may be done incrementally >>> - it is implemented and works including some optimizations enabling >>> >>> The other way I see is to try to make all current optimizations work >>> correctly with bounds by making changes in the way we instrument the >>> code. If the main problem is usage of bind_bounds and arg_bounds, >>> then we may try to avoid their usage. The obvious way to do it is to >>> use regular arg passing mechanism using call args and params in >>> function decls. The basic idea is to not modify function during >>> instrumentation, but create a new one. We may either keep the >>> original version of the function (e.g. for inline into >>> non-instrumented callers) or just leave it's declaration to be used >>> for non-instrumented calls. >>> >>> Thus, all functions may result in two nodes in cgraph. One for >>> non-instrumented version and one for instrumented version. Function >>> declaration of the instrumented function would have all bounds in it's >>> param list and instrumented call would have all bounds in it's args >>> list accordingly. >>> >>> Advantage of this approach: >>> - It would decrease number of cases when bounds flow is damaged >>> - It would allow to avoid (or at least decrease) modifications in >>> IPA passes to handle bounds correctly >>> - It would allow to unbind bounds from pointers and perform IPA >>> optimizations for bound values (e.g. if we do not know pointer value >>> but know pointer is unchecked, we may propagate it's bounds value into >>> callee using existing propagation optimization) >>> >>> Surely some doubts also exist: >>> - We have two declarations for actually the same function >>> - Most probably need some changes in PARAM_DECLs to bind bounds >>> params with pointer params >>> - If we Most probably need some modifications in passes which >>> generate new function decls (e.g. propagation or splitting) to >>> generate decl with correctly declared bounds >> >> Accidentally sent the letter before finished it :/ >> >> But seems I wrote the most of I wanted to. I know this scheme >> description with double function declarations is very basic but I >> would like to get your opinion before diving deeper. Probably current >> scheme is not so bad and we should leave basic things as it is? > > The idea of making instrumented functions clones of the original > function sounds interesting. Can you give some numbers (out of > the current implementation) on how much code ends up being > instrumented? Like when compiling GCC itself for example? I did not try to get some statistics. But current implementation instruments every function having any manipulation with a pointer which is almost every function. > > I can see there may be both instrumented and not instrumented > calls to a function and thus having two versions of the callee > might improve optimization of the non-instrumented call via > IPA optimizations that you otherwise have to disable? Currently we just always use either instrumented or not instrumented function for both instrumented and non instrumented calls. E.g. we may inline instrumented function into not instrumented and vice-versa. Agree having two versions of callee would would be preferable. It may require significant amount of memory but suppose in the most cases not instrumented function would just have no callers and may be deleted. > > Generally making dataflow explicit is always prefered. How does > making it explicit on the GIMPLE / SSA level affect RTL? (I > have not looked at all into how you deal with bounds on the RTL > level currently) In RTL bounds dataflow is explicit and follows used ABI. So, GIMPLE changes would not affect RTL. > > What model does ICC follow - the dataflow behind the backs or > the callgraph + clones? (well, more "closely" I should ask - it probably > does a third thing ...) ICC implementation details are closed for me. Thanks, Ilya > > Thanks, > Richard. > >> Thanks, >> Ilya >> >>> >>>> >>>> Jeff >>>>
diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c index e55b3f5..1c3df61 100644 --- a/gcc/ipa-split.c +++ b/gcc/ipa-split.c @@ -101,6 +101,7 @@ along with GCC; see the file COPYING3. If not see #include "gimple-pretty-print.h" #include "ipa-inline.h" #include "cfgloop.h" +#include "tree-chkp.h" /* Per basic block info. */ @@ -378,6 +379,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars, int incoming_freq = 0; tree retval; bool back_edge = false; + bitmap_iterator bi; if (dump_file && (dump_flags & TDF_DETAILS)) dump_split_point (dump_file, current); @@ -486,6 +488,28 @@ consider_split (struct split_point *current, bitmap non_ssa_vars, if (!VOID_TYPE_P (TREE_TYPE (current_function_decl))) call_overhead += estimate_move_cost (TREE_TYPE (current_function_decl)); + /* Currently bounds passing and return is not supported for + splitted functions. */ + EXECUTE_IF_SET_IN_BITMAP (current->ssa_names_to_pass, 0, i, bi) + { + if (POINTER_BOUNDS_P (ssa_name (i))) + { + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, + " Refused: need to pass bounds\n"); + return; + } + } + + if (chkp_function_instrumented_p (current_function_decl) + && chkp_type_has_pointer (TREE_TYPE (current_function_decl))) + { + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, + " Refused: need to return bounds\n"); + return; + } + if (current->split_size <= call_overhead) { if (dump_file && (dump_flags & TDF_DETAILS))