On 10 Oct 10:25, Jeff Law wrote: > On 10/10/14 08:19, Ilya Enkovich wrote: > >>So is the purpose here to expose the checks that would normally be > >>done in the mem* routines to their caller in the hopes that doing > >>so will expose redundant checks? Or is there some other reason? > > > >There are few reasons to replace instrumented string functions: > > > >1. As you said following redundant checks elimination may remove > >checks for some string functions > >2. By default functions like memcpy > >should assume pointers are copied and copy bounds. If we know > >pointers are not copied, we may use faster version with no bounds > >copy > >3. If we avoid both checks and bounds copy then it is a > >candidate for existing string function calls inlining in expand pass > Perfect. So this belongs in a comment in the code. > > >I thought tests will be added later. > Did you already post them? There's been so many patches I'm > starting to lose track :-)
I didn't post tests yet. > > For future reference, when you break a submission down into logical > hunks, including the tests in those logical hunks helps. I realize > the MPX work isn't as well suited for that kind of breakdown, but > it's worth keeping in mind. > > > I have ~250 tests to commit. > >Will check I have tests for optimizations. > Excellent. > > > BTW this particular > >optimization cann't work until we have instrumented builtin calls. > Yea, hopefully we'll get to that before close of stage1. > > >>It's a nit, but I'd tend to write that as: > >> > >>if (!fndecl_nochk) continue; > >> > >>fndecl = fndecl_nochk gimple_call_set_fndecl (stmt, fndecl); > >> > >> > >> > > > >There is one more assignment to fndecl above which makes your version > >nonequivalent. > I had assumed the gimple_call_set_fndecl was a nop if we didn't > change the fndecl. Is that not the case? Right. But (!fndecl_nochk) doesn't mean we didn't change fndecl because there is another set to fndecl above. Ilya > > >>I'm a bit surprised we don't have this kind of capability already > >>broken out. But assuming that's the case, can you go ahead and > >>break that out into its own little helper function? You don't > >>need to find all the cases where we're doing this kind of thing > >>today, just create the helper function and use it in your new > >>code. > > > >I could miss such function (looked in cfg hooks and tree-cfg.h). > >Hopefully someone will correct me if it is so. > Thanks. I suspect everyone has just done their own implementation > inline like you did. It's something I'll be keeping my eye out for > in others' code so we can funnel everyone into your new function. > ISTM many speculative optimizations are going to need that kind of > helper. > > > >Taking into account not instrumented builtin calls I suppose this > >patch goes into a next builtin related series. But here is a version > >with changes. > Yea, I think you're right. I think this is OK when the builtins are done. > > jeff