[llvm-commits] [PATCH] Teach the inline cost analysis to forward stores of constants in the caller to loads in the callee.

[Chris Lattner]

On Dec 27, 2012, at 9:53 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
>  Fix typos spotted by Joey Gouly.
> 
>  Thanks for the review!
> 
> http://llvm-reviews.chandlerc.com/D246
> 
> CHANGE SINCE LAST DIFF
>  http://llvm-reviews.chandlerc.com/D246?vs=598&id=600#toc

Hey Chandler, 

Thanks for tackling this, getting this would be a really great improvement in the inliner!  This review is based on a quick look over the patch, I apologize if I misunderstand how it's working.

My big concern about this is that you've written this in a very general way, using Alias Analysis and some amount of brute force to check the sets of values using the interval map.  I'm concerned about compile time impact and also the complexity/fragility of the code.

In practice, the inliner is going to be using BasicAA which is going to get "uniquely identified" objects and maybe TBAA information.  Also, in practice, the most common cases will be "type safe" without terrible pointer casting and stuff (I say this without actually looking at the std::vector code, I hope its true!).

Would it be enough to dramatically simplify this down to something like the following:
1. When looking at a call site, determine which arguments are known to be "uniquely identified" distinct objects.  These are your candidates for "store forwarding".  In practice, to handle the common "this" case, you should handle "uniquely identified objects" and at most one other non-uniquely identified object (commonly "this"), which still makes the aliasing trivial.
2. Do a backwards scan from the callsite to collect stores, like you're doing in collectConstantOffsetStores.  It looks like collectConstantOffsetStores can be made to bail out earlier than your scan does right now (stopping when it finds a store to an unknown-alias of the aliased "this" pointer).

3. I'd simplify your datastructure tracking stores (instead of using intervals) to only handle the "type safe" (+ memset as a followon patch, by scalarizing stores) cases, and use GEP paths (similar to what the argument promotion pass does).  What I mean is to just assume that the pointer type in the call is "correct" and look for stores to members of that LLVM IR type.  This means that the AvailableStores mapping is just "pair<Pointer+ConstantOffset> -> Constant*" since there can only be one (non-empty!) field in an IR type at a given offset, and using the argument pointer type will give you a unique and usually-good LLVM type for the constant to use. 

4. Determine which loads are candidates for forwarding in the callee, and compute which available stores get invalidated in the call.  When analyzing the callee body, you now know that the "unique" pointers coming in are non-aliasing with anything else, so the aliasing code logic is straight-forward.  To optimize for compile time, check to see if the callee is "readonly" which means it can't mutate any arguments (actually, is this true?  Can't a readonly function mutate something in the caller and then restore it before returning?).  To actually compute the invalidations (and therefore the forwardable loads) I think you might have to use a depth first walk (separate from the breadth first search the instvisitor uses) to avoid losing information.



Some minor stuff:

"get GC'ed" -> "get deallocated"
One more typo: "analyzing a particualr"

Please disable copy ctor + assignment operator on CallAnalyzer.

Is this handling "byval" arguments correctly?

-Chris