Re: [PATCH, Pointer Bounds Checker 14/x] Passes [12/n] Optimize string functions

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

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