<div dir="ltr">I think we should just revert this patch. The patch claims to not change behavior, but it clearly does. I'll follow up on the review thread with what I think is going wrong here.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 2, 2019 at 5:05 PM Jordan Rupprecht via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Fedor,<div><br><div>We're seeing some very unexpectedly large stack frame sizes as a result of this patch, e.g. from ~300 bytes to ~25kb. We're trying to reduce a test case to share. Is this kind of change expected with this patch?</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 26, 2019 at 6:24 AM Fedor Sergeev via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: fedor.sergeev<br>
Date: Wed Jun 26 06:24:24 2019<br>
New Revision: 364422<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=364422&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=364422&view=rev</a><br>
Log:<br>
[InlineCost] cleanup calculations of Cost and Threshold<br>
<br>
Summary:<br>
Doing better separation of Cost and Threshold.<br>
Cost counts the abstract complexity of live instructions, while Threshold is an upper bound of complexity that inlining is comfortable to pay.<br>
There are two parts:<br>
- huge 15K last-call-to-static bonus is no longer subtracted from Cost<br>
but rather is now added to Threshold.<br>
<br>
That makes much more sense, as the cost of inlining (Cost) is not changed by the fact<br>
that internal function is called once. It only changes the likelyhood of this inlining<br>
being profitable (Threshold).<br>
<br>
- bonus for calls proved-to-be-inlinable into callee is no longer subtracted from Cost<br>
but added to Threshold instead.<br>
<br>
While calculations are somewhat different, overall InlineResult should stay the same since Cost >= Threshold compares the same.<br>
<br>
Reviewers: eraman, greened, chandlerc, yrouban, apilipenko<br>
Reviewed By: apilipenko<br>
Tags: #llvm<br>
Differential Revision: <a href="https://reviews.llvm.org/D60740" rel="noreferrer" target="_blank">https://reviews.llvm.org/D60740</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/InlineCost.cpp<br>
llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll<br>
llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll<br>
llvm/trunk/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll<br>
llvm/trunk/test/LTO/X86/diagnostic-handler-remarks.ll<br>
llvm/trunk/test/Transforms/Inline/ARM/inline-fp.ll<br>
<br>
Modified: llvm/trunk/lib/Analysis/InlineCost.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InlineCost.cpp?rev=364422&r1=364421&r2=364422&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InlineCost.cpp?rev=364422&r1=364421&r2=364422&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/InlineCost.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/InlineCost.cpp Wed Jun 26 06:24:24 2019<br>
@@ -897,7 +897,15 @@ void CallAnalyzer::updateThreshold(CallB<br>
// and the callsite.<br>
int SingleBBBonusPercent = 50;<br>
int VectorBonusPercent = 150;<br>
- int LastCallToStaticBonus = InlineConstants::LastCallToStaticBonus;<br>
+<br>
+ int LastCallToStaticBonus = 0;<br>
+ bool OnlyOneCallAndLocalLinkage =<br>
+ F.hasLocalLinkage() && F.hasOneUse() && &F == Call.getCalledFunction();<br>
+ // If there is only one call of the function, and it has internal linkage,<br>
+ // we can allow to inline pretty anything as it will lead to size reduction<br>
+ // anyway.<br>
+ if (OnlyOneCallAndLocalLinkage)<br>
+ LastCallToStaticBonus = InlineConstants::LastCallToStaticBonus;<br>
<br>
// Lambda to set all the above bonus and bonus percentages to 0.<br>
auto DisallowAllBonuses = [&]() {<br>
@@ -970,20 +978,13 @@ void CallAnalyzer::updateThreshold(CallB<br>
}<br>
}<br>
<br>
- // Finally, take the target-specific inlining threshold multiplier into<br>
- // account.<br>
+ // Take the target-specific inlining threshold multiplier into account.<br>
Threshold *= TTI.getInliningThresholdMultiplier();<br>
<br>
SingleBBBonus = Threshold * SingleBBBonusPercent / 100;<br>
VectorBonus = Threshold * VectorBonusPercent / 100;<br>
<br>
- bool OnlyOneCallAndLocalLinkage =<br>
- F.hasLocalLinkage() && F.hasOneUse() && &F == Call.getCalledFunction();<br>
- // If there is only one call of the function, and it has internal linkage,<br>
- // the cost of inlining it drops dramatically. It may seem odd to update<br>
- // Cost in updateThreshold, but the bonus depends on the logic in this method.<br>
- if (OnlyOneCallAndLocalLinkage)<br>
- Cost -= LastCallToStaticBonus;<br>
+ Threshold += LastCallToStaticBonus;<br>
}<br>
<br>
bool CallAnalyzer::visitCmpInst(CmpInst &I) {<br>
@@ -1330,9 +1331,10 @@ bool CallAnalyzer::visitCallBase(CallBas<br>
CallAnalyzer CA(TTI, GetAssumptionCache, GetBFI, PSI, ORE, *F, Call,<br>
IndirectCallParams);<br>
if (CA.analyzeCall(Call)) {<br>
- // We were able to inline the indirect call! Subtract the cost from the<br>
- // threshold to get the bonus we want to apply, but don't go below zero.<br>
- Cost -= std::max(0, CA.getThreshold() - CA.getCost());<br>
+ // We were able to inline the indirect call! Increase the threshold<br>
+ // with the bonus we want to apply (less the cost of inlinee).<br>
+ // Make sure the bonus doesn't go below zero.<br>
+ Threshold += std::max(0, CA.getThreshold() - CA.getCost());<br>
}<br>
<br>
if (!F->onlyReadsMemory())<br>
<br>
Modified: llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll?rev=364422&r1=364421&r2=364422&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll?rev=364422&r1=364421&r2=364422&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll (original)<br>
+++ llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll Wed Jun 26 06:24:24 2019<br>
@@ -27,13 +27,13 @@<br>
; YAML-NEXT: - Caller: main<br>
; YAML-NEXT: - String: ' with '<br>
; YAML-NEXT: - String: '(cost='<br>
-; YAML-NEXT: - Cost: '-15000'<br>
+; YAML-NEXT: - Cost: '0'<br>
; YAML-NEXT: - String: ', threshold='<br>
-; YAML-NEXT: - Threshold: '337'<br>
+; YAML-NEXT: - Threshold: '15337'<br>
; YAML-NEXT: - String: ')'<br>
; YAML-NEXT: ...<br>
<br>
-; CHECK: tinkywinky inlined into main with (cost=-15000, threshold=337) (hotness: 300)<br>
+; CHECK: tinkywinky inlined into main with (cost=0, threshold=15337) (hotness: 300)<br>
<br>
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"<br>
target triple = "x86_64-scei-ps4"<br>
<br>
Modified: llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll?rev=364422&r1=364421&r2=364422&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll?rev=364422&r1=364421&r2=364422&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll (original)<br>
+++ llvm/trunk/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll Wed Jun 26 06:24:24 2019<br>
@@ -30,9 +30,9 @@<br>
; YAML-NEXT: - Caller: main<br>
; YAML-NEXT: - String: ' with '<br>
; YAML-NEXT: - String: '(cost='<br>
-; YAML-NEXT: - Cost: '-15000'<br>
+; YAML-NEXT: - Cost: '0'<br>
; YAML-NEXT: - String: ', threshold='<br>
-; YAML-NEXT: - Threshold: '337'<br>
+; YAML-NEXT: - Threshold: '15337'<br>
; YAML-NEXT: - String: ')'<br>
; YAML-NEXT: ...<br>
<br>
<br>
Modified: llvm/trunk/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll?rev=364422&r1=364421&r2=364422&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll?rev=364422&r1=364421&r2=364422&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll (original)<br>
+++ llvm/trunk/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll Wed Jun 26 06:24:24 2019<br>
@@ -19,9 +19,9 @@<br>
; YAML-NEXT: - Caller: main<br>
; YAML-NEXT: - String: ' with '<br>
; YAML-NEXT: - String: '(cost='<br>
-; YAML-NEXT: - Cost: '-15000'<br>
+; YAML-NEXT: - Cost: '0'<br>
; YAML-NEXT: - String: ', threshold='<br>
-; YAML-NEXT: - Threshold: '337'<br>
+; YAML-NEXT: - Threshold: '15337'<br>
; YAML-NEXT: - String: ')'<br>
; YAML-NEXT: ...<br>
<br>
<br>
Modified: llvm/trunk/test/LTO/X86/diagnostic-handler-remarks.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/X86/diagnostic-handler-remarks.ll?rev=364422&r1=364421&r2=364422&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/X86/diagnostic-handler-remarks.ll?rev=364422&r1=364421&r2=364422&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/LTO/X86/diagnostic-handler-remarks.ll (original)<br>
+++ llvm/trunk/test/LTO/X86/diagnostic-handler-remarks.ll Wed Jun 26 06:24:24 2019<br>
@@ -55,9 +55,9 @@<br>
; YAML-NEXT: - Caller: main<br>
; YAML-NEXT: - String: ' with '<br>
; YAML-NEXT: - String: '(cost='<br>
-; YAML-NEXT: - Cost: '-15000'<br>
+; YAML-NEXT: - Cost: '0'<br>
; YAML-NEXT: - String: ', threshold='<br>
-; YAML-NEXT: - Threshold: '337'<br>
+; YAML-NEXT: - Threshold: '15337'<br>
; YAML-NEXT: - String: ')'<br>
; YAML-NEXT: ...<br>
<br>
<br>
Modified: llvm/trunk/test/Transforms/Inline/ARM/inline-fp.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/ARM/inline-fp.ll?rev=364422&r1=364421&r2=364422&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/ARM/inline-fp.ll?rev=364422&r1=364421&r2=364422&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/Inline/ARM/inline-fp.ll (original)<br>
+++ llvm/trunk/test/Transforms/Inline/ARM/inline-fp.ll Wed Jun 26 06:24:24 2019<br>
@@ -7,7 +7,7 @@<br>
; NOFP-DAG: single not inlined into test_single because too costly to inline (cost=125, threshold=75)<br>
; NOFP-DAG: single not inlined into test_single because too costly to inline (cost=125, threshold=75)<br>
; NOFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15, threshold=75)<br>
-; NOFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15015, threshold=75)<br>
+; NOFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15, threshold=15075)<br>
; NOFP-DAG: double not inlined into test_double because too costly to inline (cost=125, threshold=75)<br>
; NOFP-DAG: double not inlined into test_double because too costly to inline (cost=125, threshold=75)<br>
; NOFP-DAG: single_force_soft not inlined into test_single_force_soft because too costly to inline (cost=125, threshold=75)<br>
@@ -16,20 +16,20 @@<br>
; NOFP-DAG: single_force_soft_fneg not inlined into test_single_force_soft_fneg because too costly to inline (cost=100, threshold=75)<br>
<br>
; FULLFP-DAG: single inlined into test_single with (cost=0, threshold=75)<br>
-; FULLFP-DAG: single inlined into test_single with (cost=-15000, threshold=75)<br>
+; FULLFP-DAG: single inlined into test_single with (cost=0, threshold=15075)<br>
; FULLFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15, threshold=75)<br>
-; FULLFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15015, threshold=75)<br>
+; FULLFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15, threshold=15075)<br>
; FULLFP-DAG: double inlined into test_double with (cost=0, threshold=75)<br>
-; FULLFP-DAG: double inlined into test_double with (cost=-15000, threshold=75)<br>
+; FULLFP-DAG: double inlined into test_double with (cost=0, threshold=15075)<br>
; FULLFP-DAG: single_force_soft not inlined into test_single_force_soft because too costly to inline (cost=125, threshold=75)<br>
; FULLFP-DAG: single_force_soft not inlined into test_single_force_soft because too costly to inline (cost=125, threshold=75)<br>
; FULLFP-DAG: single_force_soft_fneg not inlined into test_single_force_soft_fneg because too costly to inline (cost=100, threshold=75)<br>
; FULLFP-DAG: single_force_soft_fneg not inlined into test_single_force_soft_fneg because too costly to inline (cost=100, threshold=75)<br>
<br>
; SINGLEFP-DAG: single inlined into test_single with (cost=0, threshold=75)<br>
-; SINGLEFP-DAG: single inlined into test_single with (cost=-15000, threshold=75)<br>
+; SINGLEFP-DAG: single inlined into test_single with (cost=0, threshold=15075)<br>
; SINGLEFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15, threshold=75)<br>
-; SINGLEFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15015, threshold=75)<br>
+; SINGLEFP-DAG: single_cheap inlined into test_single_cheap with (cost=-15, threshold=15075)<br>
; SINGLEFP-DAG: double not inlined into test_double because too costly to inline (cost=125, threshold=75)<br>
; SINGLEFP-DAG: double not inlined into test_double because too costly to inline (cost=125, threshold=75)<br>
; SINGLEFP-DAG: single_force_soft not inlined into test_single_force_soft because too costly to inline (cost=125, threshold=75)<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>