<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META http-equiv=Content-Type content="text/html; charset=iso-8859-1">
<META content="MSHTML 6.00.6002.18591" name=GENERATOR></HEAD>
<BODY>
<DIV dir=ltr align=left><SPAN class=545295007-30042012><FONT face=Arial
color=#0000ff size=2>Is there anything more I can do to make this patch
accepted?</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=545295007-30042012><FONT face=Arial
color=#0000ff size=2></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=545295007-30042012><FONT face=Arial
color=#0000ff size=2>/Patrik Hägglund</FONT></SPAN></DIV><BR>
<DIV class=OutlookMessageHeader lang=en-us dir=ltr align=left>
<HR tabIndex=-1>
<FONT face=Tahoma size=2><B>From:</B> Patrik Hägglund H <BR><B>Sent:</B> den 26
april 2012 15:33<BR><B>To:</B> 'Chandler Carruth'; Bill Wendling<BR><B>Cc:</B>
llvm-commits@cs.uiuc.edu<BR><B>Subject:</B> RE: [llvm-commits] [PATCH] optsize
attribute on top of -Oz gives bigger code<BR></FONT><BR></DIV>
<DIV></DIV>
<DIV dir=ltr align=left><SPAN class=889002113-26042012><FONT face=Arial
color=#0000ff size=2>Sorry for the late reply.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=889002113-26042012><FONT face=Arial
color=#0000ff size=2></FONT></SPAN> </DIV>
<DIV><SPAN class=889002113-26042012>> </SPAN>I like the patch in general. I
have only one high-level comment: it needs test cases. ;] They should be pretty
easy to rig up with custom thresholds.</DIV>
<DIV><FONT face=Arial color=#0000ff size=2></FONT> </DIV>
<DIV dir=ltr align=left><SPAN class=889002113-26042012><FONT face=Arial
color=#0000ff size=2></FONT></SPAN></DIV>
<DIV><SPAN class=889002113-26042012></SPAN><FONT face=Arial><FONT
color=#0000ff><FONT size=2>M<SPAN class=889002113-26042012>y patch is for the
case when the second constructor for the Inliner class, Inliner(char &ID,
int Threshold, bool InsertLifetime), is invoked (with a low value for
Threshold). Clang invokes that constructor, but I can't see that any
tool in the llvm tree (specifically opt) is invoking that
constructor. Therefore, I can't see how to write a test case (inside
llvm/test).</SPAN></FONT></FONT></FONT><BR></DIV>
<DIV><SPAN class=889002113-26042012><FONT face=Arial color=#0000ff
size=2>/Patrik Hägglund</FONT></SPAN></DIV>
<DIV><FONT face=Arial color=#0000ff size=2></FONT> </DIV>
<DIV class=OutlookMessageHeader lang=en-us dir=ltr align=left>
<HR tabIndex=-1>
<FONT face=Tahoma size=2><B>From:</B> Chandler Carruth
[mailto:chandlerc@google.com] <BR><B>Sent:</B> den 5 april 2012
00:45<BR><B>To:</B> Bill Wendling<BR><B>Cc:</B> Patrik Hägglund H;
llvm-commits@cs.uiuc.edu<BR><B>Subject:</B> Re: [llvm-commits] [PATCH] optsize
attribute on top of -Oz gives bigger code<BR></FONT><BR></DIV>
<DIV></DIV>
<DIV class=gmail_quote>On Wed, Apr 4, 2012 at 11:17 PM, Bill Wendling <SPAN
dir=ltr><<A
href="mailto:wendling@apple.com">wendling@apple.com</A>></SPAN> wrote:<BR>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid">Hi
Patrik,<BR><BR>Thank you for running the tests. The patch looks fine to me.
Chandler may comment on the patch once it's in. If so, you can now argue that
it's beneficial. :-)<BR></BLOCKQUOTE>
<DIV><FONT face=Arial color=#0000ff size=2></FONT><BR></DIV>
<DIV>I heard that. ;]</DIV>
<DIV><FONT face=Arial color=#0000ff size=2></FONT><BR></DIV>
<DIV>I like the patch in general. I have only one high-level comment: it needs
test cases. ;] They should be pretty easy to rig up with custom
thresholds.</DIV>
<DIV><BR></DIV>
<DIV>I think there is more work to be done here, but this is a strict
improvement and gives a nice lift, so LGTM! =D</DIV>
<DIV><BR></DIV>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"><BR>-bw<BR>
<DIV>
<DIV class=h5><BR>On Apr 4, 2012, at 6:14 AM, Patrik Hägglund H
wrote:<BR><BR>> Hi Bill,<BR>><BR>> I have now taken the time to run
the test suite on r153939 from yesterday. The total object size went from
9779419 down to 9634074, when my patch was applied.<BR>><BR>> In both
builds, I replaced 'OPTFLAGS := -O3' with 'OPTFLAGS := -Oz' in Makefile.rules
in the test-suite directory, and executed 'make TEST=simple'. I then ran 'size
-t `find -name \*.simple`'. The diff is attached.<BR>><BR>> BTW, is
there any build bot logs that can be used to see the current status for test
suite executions? For example, I didn't got the sphereflake test to pass (and
one other test, which I have forgotten).<BR>><BR>> Here is the updated
patch:<BR>><BR>> diff --git a/lib/Transforms/IPO/Inliner.cpp
b/lib/Transforms/IPO/Inliner.cpp<BR>> index 8a9d149..4e13596 100644<BR>>
--- a/lib/Transforms/IPO/Inliner.cpp<BR>> +++
b/lib/Transforms/IPO/Inliner.cpp<BR>> @@ -196,19 +196,23 @@ static bool
InlineCallIfPossible(CallSite CS, InlineFunctionInfo &IFI,<BR>>
}<BR>><BR>> unsigned Inliner::getInlineThreshold(CallSite CS) const
{<BR>> - int thres = InlineThreshold;<BR>> + int thres =
InlineThreshold; // -inline-threshold or else selected by<BR>> +
// overall opt level<BR>><BR>> - // Listen to
optsize when -inline-limit is not given.<BR>> + // If
-inline-threshold is not given, listen to the optsize attribute when
it<BR>> + // would decrease the threshold.<BR>> Function
*Caller = CS.getCaller();<BR>> - if (Caller &&
!Caller->isDeclaration() &&<BR>> -
Caller->hasFnAttr(Attribute::OptimizeForSize) &&<BR>> +
bool OptSize = Caller && !Caller->isDeclaration()
&&<BR>> +
Caller->hasFnAttr(Attribute::OptimizeForSize);<BR>> + if
(OptSize && OptSizeThreshold < thres &&<BR>>
InlineLimit.getNumOccurrences() == 0)<BR>>
thres = OptSizeThreshold;<BR>><BR>> - // Listen to inlinehint when
it would increase the threshold.<BR>> + // Listen to the inlinehint
attribute when it would increase the threshold.<BR>> Function
*Callee = CS.getCalledFunction();<BR>> - if (HintThreshold > thres
&& Callee && !Callee->isDeclaration() &&<BR>> -
Callee->hasFnAttr(Attribute::InlineHint))<BR>> +
bool InlineHint = Callee && !Callee->isDeclaration()
&&<BR>> +
Callee->hasFnAttr(Attribute::InlineHint);<BR>> + if
(InlineHint && HintThreshold > thres)<BR>> thres =
HintThreshold;<BR>><BR>> return thres;<BR>><BR>> /Patrik
Hägglund<BR>><BR>> -----Original Message-----<BR>> From: Patrik
Hägglund H<BR>> Sent: den 22 mars 2012 08:57<BR>> To: 'Bill
Wendling'<BR>> Cc: <A
href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</A><BR>>
Subject: RE: [llvm-commits] [PATCH] optsize attribute on top of -Oz gives
bigger code<BR>><BR>>> Could you give some metrics to go along with
the patch? You should be able to measure the impact on the LLVM test
suite.<BR>><BR>> Well, the few times I have tried to execute the test
suite, it have failed in one way or another. (On SLED 11 and Debian unstable).
But maybe its time to give it another try.<BR>><BR>> BTW, what's the
purpose of the metrics? My thinking was that this is more an issue about how
you define the optsize attribute. In my opinion adding optisize should not
make any impact on the code size when using clang -Os or -Oz. At least
the code should not be larger, but not smaller either. That is, the code
should be identical. That is what this patch tries to fix (with the regard to
the inliner). If the code shouldn't be identical, exactly what is the
definiton of the optsize attribute, relative to clang -Os and -Oz (or other
potential users of the LLVM libraries passing similar
information)?<BR>><BR>>> A comment on the patch
itself:<BR>><BR>> I like consistency. The logic mirrors the order in the
comment, and InlineLimit.getNumOccurrences() > 0 is used because it is also
used in the constructor. But I can change that part if you
like.<BR>><BR>> /Patrik Hägglund<BR>><BR>> -----Original
Message-----<BR>> From: Bill Wendling [mailto:<A
href="mailto:wendling@apple.com">wendling@apple.com</A>]<BR>> Sent: den 22
mars 2012 08:10<BR>> To: Patrik Hägglund H<BR>> Cc: <A
href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</A><BR>>
Subject: Re: [llvm-commits] [PATCH] optsize attribute on top of -Oz gives
bigger code<BR>><BR>> Hi Patrik,<BR>><BR>> The patch looks okay
overall. Could you give some metrics to go along with the patch? You should be
able to measure the impact on the LLVM test suite.<BR>><BR>> A comment
on the patch itself:<BR>><BR>> + if
(!(InlineLimit.getNumOccurrences() > 0) && OptSize &&
OptSizeThreshold < thres)<BR>><BR>> The logic could be made better
(and more readable). Please use this instead:<BR>><BR>> + if
(OptSize && OptSizeThreshold < thres &&
InlineLimit.getNumOccurrences() == 0)<BR>><BR>> -bw<BR>><BR>> On
Mar 20, 2012, at 6:30 AM, Patrik Hägglund H wrote:<BR>><BR>>>
Hi,<BR>>><BR>>> I found that using clang -Oz, used as a backend
for .ll files, combined with using the optsize attribute, then optsize
_increased_ the code size.<BR>>><BR>>> The base inline threshold
was 25 with clang -Oz, but 75 when adding the optsize
attribute.<BR>>><BR>>> This patch fix this to only change the
inline threshold for a function with optsize if it lowers the base
threshold.<BR>>><BR>>> /Patrik Hägglund<BR>>><BR>>>
<optsize_inliner.diff>_______________________________________________<BR>>>
llvm-commits mailing list<BR>>> <A
href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</A><BR>>>
<A href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits"
target=_blank>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</A><BR>><BR></DIV></DIV>>
<size.diff><BR>
<DIV class=HOEnZb>
<DIV
class=h5><BR><BR>_______________________________________________<BR>llvm-commits
mailing list<BR><A
href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</A><BR><A
href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits"
target=_blank>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</A><BR></DIV></DIV></BLOCKQUOTE></DIV><BR></BODY></HTML>