[PATCH] D12979: Avoid inlining in exception handling context

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Transforms/IPO/PruneEH.cpp:362
@@ +361,3 @@
+  // call void @__cxa_throw(i8* %exception, .. )
+  for (User *FnU : FnThrowE->users()) {
+    auto *ThrowCall = dyn_cast<CallInst>(FnU);
----------------
junbuml wrote:
> hfinkel wrote:
> > junbuml wrote:
> > > hfinkel wrote:
> > > > junbuml wrote:
> > > > > hfinkel wrote:
> > > > > > Why are you looking for users of __cxa_throw? Can we make the assumption that all user of __cxa_allocate_exception are essentially cold, and just look for the users of return values of that function?
> > > > > > 
> > > > > I'm trying to find all actual calls for __cxa_throw() by looking at the users of FnThrowE where an exception is thrown. 
> > > > > 
> > > > > Here, I specifically check the exception allocated to be thrown (__cxa_throw()) in throw statement, which should be cold unless the program logic highly rely on throw statement.
> > > > > 
> > > > > When a exception handling class is created in throw statement, 
> > > > > the constructor must take the exception used in __cxa_throw() as its first parameter.
> > > > > 
> > > > > Please correct me if I miss something.
> > > > > 
> > > > I understand what you're doing, but my question is this: Is the call to __cxa_allocate_exception itself cold? If so, perhaps that's all we need to find. Maybe looking for the __cxa_throw calls is an unnecessary complication.
> > > > 
> > > I believe __cxa_allocate_exception() itself must be cold in almost all normal c++ program. But, I just wanted to make sure the exception allocated is actually used for throwing. If checking __cxa_throw() doesn't make it too complicated, I believe there is no harm with it. I can also leave FIXME about it.  Please let me know your thought!
> > > I believe cxa_allocate_exception() itself must be cold in almost all normal c++ program. But, I just wanted to make sure the exception allocated is actually used for throwing. If checking cxa_throw() doesn't make it too complicated, I believe there is no harm with it. I can also leave FIXME about it. Please let me know your thought!
> > 
> > Okay.
> > 
> > Let me also ask something I should have asked in the very beginning: Why are you doing this here and not in Clang? It seems that, in Clang, it would be simple to have a piece of state that records whether we're in a throw statement or a catch block, and marks the call sites appropriately.
> I just wanted to place it somewhere exception handling specific pass, instead of directly touching inliner. If you believe Clang (ItaniumCXXABI.cpp ?) is better place I can do the same thing in there. 
>  
This might be really off base, but here's what I was thinking about last night...

We're trying to mark all callsites within throw statements (and catch blocks) as cold (noinline). This seems analogous to me to how we handle setting instruction fast-math flags, where we give the IRBuilder a default state, and change it based on scope information.

 1. Give IRBuilder a new default state argument (along with DefaultFPMathTag/FMF) to indicate that all calls are cold (noinline).

 2. In Clang, update CodeGenFunction::EmitCXXThrowExpr in lib/CodeGen/CGException.cpp to set/reset this default IRBuilder state around the rest of what it does.

 3. To make the catch regions also have cold callsites, edit CodeGenFunction::ExitCXXTryStmt, and set/reset the default IRBuilder state around the loop over Handlers.

This seems like it might be both more general and simpler.



http://reviews.llvm.org/D12979