> I think this actually makes it less general. You would presumably perform > different inlining for: > > throw f(x, y); > > versus > > auto k = f(x, y); > throw k;
We need to differentiate between these two. For the second case, we should not add any attribute because itÂ’s not invoked in the EH region and it could have any other purposes other than exception handling. I believe we need to specifically handle only the case where we can guarantee that the call is invoked only in exception handling context. > That's not something we can really do from the frontend, because small > constructors often don't look small until they themselves have been > optimized. I agree. If we need to add check for callee size, doing this in frontend may not be the good idea. But it should be done before inliner. > Would it be sufficient to mark just the invocation of __cxa_throw (or its > equivalent) as cold? If the optimizer can't infer from that that the code > leading up to the call is also cold, we should fix that in the LLVM-side > analysis rather than working around it in the frontend. Basically, current Inliner doesn't have any impact with Attribute::Cold because ColdThreshold and InlineLimit are the same by default. I understand noinline looks somewhat strong decision. But, even with noinline, I believe we could expect more pros than cons. In performance perspective, by avoiding inlining in throw statement, we could open up more inline opportunities for functions containing throw statements. For example, below small function could not be inlined in its many callsites if the constructor of IndexOutOfBoundsException is non-trivial and inlined first in elementAt(). int elementAt(int idx) { if (idx >= limit) throw IndexOutOfBoundsException(idx, limit); return Array[idx]; } Pretty much same thing could actually happen in many c++ programs, and I found the actual cases in spec2006/xalancbmk. In most case we also don't need to increase code size by inlining constructors invoked in throw statement, especially when there is a hierarchy of exception handling classes. The only downsides I can think of is the case where the constructor is very small so that inlining it is profitable for size. My suggestion for this is to move this implementation back to PruneEH.cpp so that we can check the callee size right before the inliner. I may add the noinline only when the constructor is large enough. > On Wed, Oct 7, 2015 at 3:10 PM, Jun Bum Lim via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> junbuml added a comment. >> >> Thanks Richard for the comment. >> >> Initially, I intended to implement this in inliner by checking if a >> callsite is in exception handling regions. However, I decided not to >> implement this in inliner because this kind of check should be performed >> for all callsites if we implement it in inliner. >> >> Instead of directly adding complexity in inliner, I implemented this in >> PruneEH.cpp (http://reviews.llvm.org/D12979) because this is very >> specific to exception handling regions. In this patch, I tried to mark >> all >> callsites invoked from throw statements as cold (noinline) by looking up >> users of __cxa_throw() and __cxa_allocate_exception(). We had many >> discussions and finally ended up to implement the same thing in clang to >> be >> more general and simpler as Hal suggested in >> http://reviews.llvm.org/D12979. >> > > I think this actually makes it less general. You would presumably perform > different inlining for: > > throw f(x, y); > > versus > > auto k = f(x, y); > throw k; > > which doesn't really seem defensible. > > >> As you point out, it should be done by influencing inline cost heurisic, >> so I believe Attribute::Cold is the right attribute to be added here. >> However, as I FIXMEed, the current ColdThreshold is not tuned yet >> (r200898). So for now, I add both cold and noinline. >> >> Regarding code size, I believe not inlining contractor calls in throw >> statements could be potentially more helpful for code size in many >> cases. >> If inlining very small callsites in throw statements could be issue, >> then >> we may be able to check if callee is smaller than some threshold to >> avoid >> adding the attributes (cold and noinline). > > > That's not something we can really do from the frontend, because small > constructors often don't look small until they themselves have been > optimized. > > Would it be sufficient to mark just the invocation of __cxa_throw (or its > equivalent) as cold? If the optimizer can't infer from that that the code > leading up to the call is also cold, we should fix that in the LLVM-side > analysis rather than working around it in the frontend. > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits