Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml abandoned this revision. junbuml added a comment. Abandon this based the last comment in http://reviews.llvm.org/D15289. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. Just ping to see if there is any objection about adding the extra check for CallSites in EHRs in inliner. I will be happy to hear any opinion, suggestion, or objection. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. If we want to add a check for CallSites in EHRs in inliner, we may be able to borrow things done in BranchProbabilityInfo::calcColdCallHeuristics, but for exception handing intrinsics, not for cold, and make getInlineThreshold() return a lower threshold so that we can be conservative for calls in EHR. Considering that inliner will be hooked with BPI / BFI with the new pass manager, as Hal mentioned above we may need to mark the exception handling intrinsics as cold so that we can allow BranchProbabilityInfo::calcColdCallHeuristics to set weight properly for blocks that branch to exception regions. I believe we can do this at prune-eh if front-end don’t do that, but it could be done later. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
manmanren added a subscriber: manmanren. manmanren added a comment. Inliner currently does not include analysis passes such as BPI and BFI. With the new pass manager, we should be able to hook up inliner with BFI (and we can handle throw in BFI). Before that, maybe we can add this as part of inlining analysis? http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. Thanks Richard for your comment ! If the frond-end is not a good to place for this, I think there are two places we can consider : inliner or prune-eh. 1. In inliner, we can directly check if a CallSite branches an exception region, and then make getInlineThreshold() return a lower threshold. 2. If we want to avoid adding the additional check in inliner, we can move back to PruneEH.cpp(http://reviews.llvm.org/D12979). If NoInline is too strong to use, then I want to suggest to introduce a new attribute and allow ininliner to check the new attribute and decide lower inline threshold for callsites in exception handling regions. Please let me know any opinion or any better suggestion. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
rsmith added a comment. I am not convinced that it's reasonable to put inlining heuristics into clang's IR generation. This will cause maintenance problems for the inliner in the future (anyone tuning the inlining heuristics and thresholds will need to be aware of this, and clang will behave differently from other frontends in this regard). I don't really see a need for clang's IR to change in order to support the inliner in this regard -- the inliner can already determine that a call to a `noreturn` function can only exit via unwinding, and can already determine which code is only reachable via landingpads. That seems like enough information for all the cases you're addressing here. (If I remember correctly, we already have a heuristic that treats calls followed by `unreachable` as being cold; perhaps we could extend that to also look for invokes that branch to `unreachable`, if it doesn't already handle that case.) If we handle this at that level, we'll also handle cases such as a user-defined function whose purpose is to throw an exception, and we'll treat the code leading to the throw the same regardless of whether it's within the operand of the //throw-expression//. `noinline` seems like too strong of a constraint for this case. There are calls that we really want to inline, even if they occur in cold regions (because doing so will reduce code size), and calls that we must inline for correctness (calls to `always_inline` functions). If the `cold` attribute isn't working properly, that's an inliner bug and should be fixed in the inliner, not worked around here. If you want to change the inlining heuristics to make a different choice here, it seems to me that you should make that change in the inliner. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
mcrosier added a comment. @chandlerc: Adding Chandler in case he has an opinion on how to move forward or how we could go about tuning the cold threshold. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. The basic idea of this change is to avoid inlining callsites invoked in exception handling regions (EHR) so that we can reduce code size blow-up in very cold regions and indirectly increase inline opportunities for functions containing exception handling code. I think the best way to implement this is influencing to the inline heuristic by smoothly decreasing the threshold for the callsites in EHR. From this perspective, Attribute::Cold seems to be the right attribute to be added. However, the current ColdThreshold is still not tuned yet (r200898) and too high to conservatively perform inlining in EHR. As a work-around, in my current implementation, both Cold and NoInline are added to the callsites. I understand that the NoInline is somewhat strong attribute to be added, but I don't think there is any negative impact on performance unless the execution logic really depends on exception handling flows, which is rare. The only downsides I can think of is the case where the callee is very small so that inlining it is profitable for size, but the impact must be minor. If using NoInline is too strong to use, another work-around could be introducing a new attribute something like "ColdInEHR", and then we decrease the inline threshold for the callsites marked with this attribute. OptSizeThreshold(75) could be considered to be a candidate for the default threshold. Unless there is a strong objection about the basic idea, I want to move forward and close this issue as soon as possible. Please let me know any opinion. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. I see what you mean. Now, I doubt if BranchProbabilityInfo::calcColdCallHeuristics() set the Cold before inliner. As far as I check, BranchProbabilityInfo is executed after inliner. Another issue is that even if we can add the Cold in callsites in exception handling regions before inliner, the default ColdThreshold (225) in the inliner is still not tuned (r200898), so I cannot see the expected performance improvement only with the Cold. If we don't have any plan to tune the ColdThreshold in near future, we may need to use other ways to conservatively inline in exception handling regions. So, for example, in this patch I simply added both Cold and NoInline. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
hfinkel added a comment. In http://reviews.llvm.org/D13304#276660, @junbuml wrote: > Did you mean to add the Cold in the exception handling region itself instead > of callsite in throw statements ? We already have BranchProbabilityInfo::calcColdCallHeuristics, and so adding it to the callsite should be sufficient. Existing infrastructure should take care of the rest. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. Did you mean to add the Cold in the exception handling region itself instead of callsite in throw statements ? http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
hfinkel added a comment. In http://reviews.llvm.org/D13304#269049, @junbuml wrote: > I just want to ping one more time to see if there is any objection about the > basic idea of this change. If the basic idea itself is acceptable, then I > want to find the best way to get idea in. > > Please let me know any new suggestion or any opinion about moving this > implementation back to backend (maybe in PrunceEH.cpp) with the minimum > callee size check to avoid blindly marking noinlines on all callsites. I will > be happy to hear any opinion. It seems like we want two things here: 1. Mark the exception-handling intrinsics as cold 2. Have the inliner not inline things in cold regions What is preventing us from doing this now? http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. I just want to ping one more time to see if there is any objection about the basic idea of this change. If the basic idea itself is acceptable, then I want to find the best way to get idea in. Please let me know any new suggestion or any opinion about moving this implementation back to backend (maybe in PrunceEH.cpp) with the minimum callee size check to avoid blindly marking noinlines on all callsites. I will be happy to hear any opinion. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. Just want to hear if there is any opinion about moving this implementation back to PrunceEH.cpp like http://reviews.llvm.org/D12979 and adding the minimum callee size check to avoid marking noinlines on trivial constrictor calls. Please let me know any objection or suggestion. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
Yes, these are functionally equivalent. However, the difference between these two is that the call to f() in 1st is in sub-expression of throw statement, while the call in 2nd is not. If the call is in the sub-expression of throw statements, we can guarantee that the call is only invoked to be thrown. I just wanted to chase this clear case. > On Thu, Oct 8, 2015 at 9:54 AM, wrote: > >> > 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. > > > Modulo copying the value returned by f, those two are equivalent, and it's > entirely reasonable to want to refactor one into the other. Such > refactoring should not affect the optimizer's behavior on this code. Why > do > you think they should be treated differently? > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
On Thu, Oct 8, 2015 at 9:54 AM, wrote: > > 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. Modulo copying the value returned by f, those two are equivalent, and it's entirely reasonable to want to refactor one into the other. Such refactoring should not affect the optimizer's behavior on this code. Why do you think they should be treated differently? ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
> 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 its 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
Re: [PATCH] D13304: Avoid inlining in throw statement
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
Re: [PATCH] D13304: Avoid inlining in throw statement
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. 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). http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
rsmith added a subscriber: rsmith. rsmith added a comment. This seems like something that's much better handled by the inliner. We should presumably treat all the code that leads up to the throw as being cold, not just the invocation of the throw-expression itself, and it seems like it should be straightforward for the inliner to check whether a call invariably leads to an `unreachable` or an unwind. Also, blindly marking the calls as noinline doesn't seem like a good idea at all. This should just factor into the inline cost heuristics, and shouldn't stop us inlining in cases where doing so is obviously going to cause a code size reduction. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml added a comment. Is there any comment on this change? Note that with change, I observed about 6% performance improvement in spec2006/xalancbmk. I will address any comment to get this in. http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
junbuml updated this revision to Diff 36551.
junbuml added a comment.
Just minor cleaning. Please let me know any comment.
http://reviews.llvm.org/D13304
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGException.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/throw-expressions.cpp
Index: test/CodeGenCXX/throw-expressions.cpp
===
--- test/CodeGenCXX/throw-expressions.cpp
+++ test/CodeGenCXX/throw-expressions.cpp
@@ -112,3 +112,14 @@
// CHECK: ret i32* @val
return cond ? val : ((throw "foo"));
}
+
+// CHECK-LABEL: _Z5test9v
+// CHECK: invoke void @_ZN2EHC1Ev(%class.EH* %0) [[ATTR_NUM:#[0-9]+]]
+// CHECK: attributes [[ATTR_NUM]] = { cold noinline }
+class EH {
+public :
+ EH();
+};
+void test9() {
+ throw EH();
+}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -283,6 +283,11 @@
/// finally block or filter expression.
bool IsOutlinedSEHHelper;
+ // True if the current insertion point is in cold regions (e.g., exception
+ // handling regions). As of now, this flag is true only when handling throw
+ // statements.
+ bool IsColdRegion;
+
const CodeGen::CGBlockInfo *BlockInfo;
llvm::Value *BlockPointer;
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -44,7 +44,7 @@
CapturedStmtInfo(nullptr),
SanOpts(CGM.getLangOpts().Sanitize), IsSanitizerScope(false),
CurFuncIsThunk(false), AutoreleaseResult(false), SawAsmBlock(false),
- IsOutlinedSEHHelper(false),
+ IsOutlinedSEHHelper(false), IsColdRegion(false),
BlockInfo(nullptr), BlockPointer(nullptr),
LambdaThisCaptureField(nullptr), NormalCleanupDest(nullptr),
NextCleanupDestIndex(1), FirstBlockInfo(nullptr), EHResumeBlock(nullptr),
Index: lib/CodeGen/CGException.cpp
===
--- lib/CodeGen/CGException.cpp
+++ lib/CodeGen/CGException.cpp
@@ -400,6 +400,11 @@
void CodeGenFunction::EmitCXXThrowExpr(const CXXThrowExpr *E,
bool KeepInsertionPoint) {
+ // While handling the throw statement, inform that the insertion point is in
+ // cold regions so that we could perform cold region specific IR generation.
+ // For example, the NoInline attribute could be added in CallSites in throw
+ // statements.
+ IsColdRegion = true;
if (const Expr *SubExpr = E->getSubExpr()) {
QualType ThrowType = SubExpr->getType();
if (ThrowType->isObjCObjectPointerType()) {
@@ -417,6 +422,10 @@
// to leave ourselves at a valid insertion point.
if (KeepInsertionPoint)
EmitBlock(createBasicBlock("throw.cont"));
+
+ IsColdRegion = false;
+ // FIXME: Similarly, we could set IsColdRegion for catch blocks in
+ // ExitCXXTryStmt().
}
void CodeGenFunction::EmitStartEHSpec(const Decl *D) {
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3464,6 +3464,27 @@
Attrs.addAttribute(getLLVMContext(), llvm::AttributeSet::FunctionIndex,
llvm::Attribute::NoInline);
+ // As of now, IsColdRegion is true only while handling throw statements.
+ // It might be reasonable to avoid inlining CallSites invoked in exception
+ // handling context so that we can reduce code size blow-up in EH regions
+ // as well as indirectly increase inline opportunities for unwinding
+ // functions containing exception handling code.
+ if (IsColdRegion) {
+// FIXME: We add both NoInline and Cold because the inline cold-threshold
+// is not tuned yet (r200898). As of now, considering that CallSites in
+// exception handling regions are very cold is not unreasonable even without
+// profiling, and avoiding inlining in exception handling region may not
+// have significant impacts on performance unless a program execution logic
+// really depends on exception handling flows. However, when the inline
+// cold-threshold is tuned, we may need to remove NoInline here so that we
+// can allow a trivial constructor to be inlined.
+Attrs =
+Attrs.addAttribute(getLLVMContext(), llvm::AttributeSet::FunctionIndex,
+ llvm::Attribute::NoInline);
+Attrs =
+Attrs.addAttribute(getLLVMContext(), llvm::AttributeSet::FunctionIndex,
+ llvm::Attribute::Cold);
+ }
CS.setAttributes(Attrs);
CS.setCallingConv(static_cast(CallingConv));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13304: Avoid inlining in throw statement
mcrosier added a comment. FWIW, a llvm based solution was discussed in http://reviews.llvm.org/D12979, but the clang solution is obviously more robust and easier to implement. Comment at: lib/CodeGen/CodeGenFunction.h:287 @@ +286,3 @@ + // True if the current insertion point is in cold regions (e.g., exception + // handling regions). As of now, this flag is ture only when handling throw + // statement. ture -> true http://reviews.llvm.org/D13304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D13304: Avoid inlining in throw statement
junbuml created this revision.
junbuml added reviewers: hfinkel, mcrosier, reames, ashutosh.nema, majnemer.
junbuml added subscribers: gberry, cfe-commits.
It might be reasonably to avoid inlining CallSites invoked in
exception handling context so that we can reduce code size blow-up in
EH regions as well as indirectly increase inline opportunites for unwinding
functions containing exception handling code.
In this change, the NoInline attribute is added in CallSites
invoked specifically by the throw statement.
http://reviews.llvm.org/D13304
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGException.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/throw-expressions.cpp
Index: test/CodeGenCXX/throw-expressions.cpp
===
--- test/CodeGenCXX/throw-expressions.cpp
+++ test/CodeGenCXX/throw-expressions.cpp
@@ -112,3 +112,14 @@
// CHECK: ret i32* @val
return cond ? val : ((throw "foo"));
}
+
+// CHECK-LABEL: _Z5test9v
+// CHECK: invoke void @_ZN2EHC1Ev(%class.EH* %0) [[ATTR_NUM:#[0-9]+]]
+// CHECK: attributes [[ATTR_NUM]] = { cold noinline }
+class EH {
+public :
+ EH();
+};
+void test9() {
+ throw EH();
+}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -283,6 +283,11 @@
/// finally block or filter expression.
bool IsOutlinedSEHHelper;
+ // True if the current insertion point is in cold regions (e.g., exception
+ // handling regions). As of now, this flag is ture only when handling throw
+ // statement.
+ bool IsColdRegion;
+
const CodeGen::CGBlockInfo *BlockInfo;
llvm::Value *BlockPointer;
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -44,7 +44,7 @@
CapturedStmtInfo(nullptr),
SanOpts(CGM.getLangOpts().Sanitize), IsSanitizerScope(false),
CurFuncIsThunk(false), AutoreleaseResult(false), SawAsmBlock(false),
- IsOutlinedSEHHelper(false),
+ IsOutlinedSEHHelper(false), IsColdRegion(false),
BlockInfo(nullptr), BlockPointer(nullptr),
LambdaThisCaptureField(nullptr), NormalCleanupDest(nullptr),
NextCleanupDestIndex(1), FirstBlockInfo(nullptr), EHResumeBlock(nullptr),
Index: lib/CodeGen/CGException.cpp
===
--- lib/CodeGen/CGException.cpp
+++ lib/CodeGen/CGException.cpp
@@ -400,6 +400,11 @@
void CodeGenFunction::EmitCXXThrowExpr(const CXXThrowExpr *E,
bool KeepInsertionPoint) {
+ // While handling the throw statement, inform that the insertion point is in
+ // cold regions so that we could perform cold region specific IR generation.
+ // For example, the NoInline attribute could be added in CallSites in throw
+ // statements.
+ IsColdRegion = true;
if (const Expr *SubExpr = E->getSubExpr()) {
QualType ThrowType = SubExpr->getType();
if (ThrowType->isObjCObjectPointerType()) {
@@ -417,6 +422,9 @@
// to leave ourselves at a valid insertion point.
if (KeepInsertionPoint)
EmitBlock(createBasicBlock("throw.cont"));
+
+ IsColdRegion = false;
+ // FIXME: Similarly, we could set IsColdRegion for catch blocks in ExitCXXTryStmt().
}
void CodeGenFunction::EmitStartEHSpec(const Decl *D) {
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3464,6 +3464,27 @@
Attrs.addAttribute(getLLVMContext(), llvm::AttributeSet::FunctionIndex,
llvm::Attribute::NoInline);
+ // As of now, IsColdRegion is true only while handling throw statements.
+ // It might be reasonable to avoid inlining CallSites invoked in exception
+ // handling context so that we can reduce code size blow-up in EH regions
+ // as well as indirectly increase inline opportunities for unwinding
+ // functions containing exception handling code.
+ if (IsColdRegion) {
+// FIXME: We add both NoInline and Cold because the inline cold-threshold
+// is not tuned yet (r200898). As of now, considering that CallSites in
+// exception handling regions are very cold is not unreasonable even without
+// profiling, and avoiding inlining in exception handling region may not have
+// significant impacts on performance unless a program execution logic
+// really depends on exception handling flows. However, when the inline
+// cold-threshold is tuned, we may need to remove NoInline here so that we
+// can allow a trivial constructor to be inlined.
+Attrs =
+Attrs.addAttribute(getLLVMContext(), llvm::AttributeSet::FunctionIndex,
+ llvm::Attribute::NoInline);
+Attrs =
+Attrs.addAttribute(g
