<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 1, 2016 at 10:39 PM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><span class=""><br><div><blockquote type="cite"><div>On Nov 1, 2016, at 10:01 PM, Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>> wrote:</div><br class="m_-1215484276421138568Apple-interchange-newline"><div><br class="m_-1215484276421138568Apple-interchange-newline"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, Nov 1, 2016 at 8:06 PM, Mehdi AMINI<span class="m_-1215484276421138568Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span><span class="m_-1215484276421138568Apple-converted-space"> </span><wbr>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">mehdi_amini added a comment.<br><br>(Add inline the answer to David, so that there is the relevant context)<br><br><br><br>================<br>Comment at: lib/Analysis/InlineCost.cpp:93<wbr>8<br>-      if (!isa<InlineAsm>(CS.getCalledV<wbr>alue()))<br>-        Cost += InlineConstants::CallPenalty;<br>     }<br>----------------<br><span>>>! In D24338, @davidxl wrote:<br>> Do you mean parameter passing? That should be counted independently  by the instruction visitor.<br><br></span>No, I don’t mean the parameters, the visitor account for one instruction per parameter just before this indeed.<br>However a call has other extra-cost on top of a regular instruction, even without any argument: it splits live-ranges and may cause spills around it to preserve registers that aren't callee-saved.<br><br>In this sense, it make sense that a call can "cost" more than a normal instruction.<br></blockquote><div><br></div><div>yes, inlining can lead to increased register pressure leading to use of caller save registers (and spills around new callsites). However I think this should not be blindly applied -- better tied with register pressure analysis.</div></div></div></blockquote><br></div></span><div>I’m not sure we’re talking on the same level here: I don’t claim the the register pressure will increase in the caller after inlining here, or at least not because of call instructions in the callee.</div><div>My assumption (possibly incorrect), is that the current inliner cost model trades off the cost of the call site with the increased code size resulting from the inlining.</div><div><br></div><div>When we consider the overall “cost” of a callee: we a cost per-instruction in the visitor. The question is what is this cost representing? From a code size point of view, it make sense to add a penalty for call instructions because they have potentially more impacts (spills is an example that is specific to calls).</div></div></blockquote><div><br></div><div>From the way it is currently implemented, it is likely the cost is to model the size increase. Yes, spills can result in size increase too -- but my point is that size cost of spills can not be unconditionally applied.  </div><div> </div><div>To compute runtime cost/benefit, the 'runtime cost diff's of the callee and inline instance of the callee needs to be computed (with the help of profile info). Easwaran has a speed-up analysis patch that does that.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>I don’t believe the patch as-is demonstrate that removing this penalty is the right thing to do: I’d like to see first how it compares to:</div><div>1) simply starting by deducing the cost of one call (the call-site considered for inlining) ;</div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>it is included in this patch, but should be split and committed first: that’s mostly a “bug fix”.</div></div></blockquote><div><br></div><div>this sounds reasonable.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>2) simply increasing the threshold.</div><div><br></div></div></blockquote><div>This is a different tuning IMO.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div></div><div>Without these additional baseline, it is not clear to me that the measurement is sound: we can very well obtain better result because we will now inline a function that has multiple calls, while there can exist similar function without many calls but a bit more arithmetic that would also be profitable to inline.</div></div></blockquote><div><br></div><div>Or do this: split the current CallPenalty into two parameters:</div><div>* CallPenalty which is used in patch  #1 you suggested -- i.e, the value to be deducted from the cost of  callsite to be inlined</div><div>* CallInstrCost -- this value is used to model the 'size cost' of call instruction (which considers potential spilling code). The value can be tuned independently -- lowed below 25 but higher than 5 (the default instruction cost).</div><div><br></div><div>David</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>— </div><span class="HOEnZb"><font color="#888888"><div>Mehdi</div><div><br></div></font></span></div></blockquote></div><br></div></div>