<div class="gmail_quote">On Mon, May 7, 2012 at 6:34 AM, Patrik Hägglund H <span dir="ltr"><<a href="mailto:patrik.h.hagglund@ericsson.com" target="_blank">patrik.h.hagglund@ericsson.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<u></u>
<div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">Hi,</font></span></div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">Any progress?</font></span></div></div></blockquote><div><br></div><div>Due to other pressing issues, no I've not had time to come up with anything. =/ Sorry. I don't know when I would have time as this isn't something likely to quickly return to my priority list. It will eventually, but you seem to have more immediate needs.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="im">
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">I wrote:</font></span></div>> I can't see that any tool
in the llvm tree (specifically opt) is invoking that constructor.<span></span><br>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff"></font></span> </div>
</div><div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">Can you confirm or refute this?</font></span></div></div></blockquote><div><br></div><div>I've no idea, I'd have to go read a pile of code to figure it out. That said, if you've read the pile of code, I tend to believe you that we don't have such a tool today.</div>
<div><br></div><div>Maybe look at adding support for this to opt yourself if you've a need to enhance this aspect of LLVM? I think that's the fastest path toward progress.</div><div><br></div><div>-Chandler</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">/Patrik Hägglund</font></span></div><br>
<div lang="en-us" dir="ltr" align="left">
<hr>
<font face="Tahoma"><div class="im"><b>From:</b> Chandler Carruth
[mailto:<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>] <br></div><b>Sent:</b> den 30 april 2012
12:21<div><div class="h5"><br><b>To:</b> Bill Wendling<br><b>Cc:</b> Patrik Hägglund H;
<<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>><br><b>Subject:</b> Re: [llvm-commits] [PATCH]
optsize attribute on top of -Oz gives bigger code<br></div></div></font><br></div><div><div class="h5">
<div></div>
<p>I agree, we need some testing of this logic. I can try to figure out how
myself, but it will be a few days I suspect. </p>
<div class="gmail_quote">On Apr 30, 2012 3:19 AM, "Bill Wendling" <<a href="mailto:wendling@apple.com" target="_blank">wendling@apple.com</a>> wrote:<br type="attribution">
<blockquote class="gmail_quote" style="PADDING-LEFT:1ex;MARGIN:0px 0px 0px 0.8ex;BORDER-LEFT:#ccc 1px solid">Sorry.
I think if you can cobble together some test cases, then it would be
good.<br><br>-bw<br><br>On Apr 30, 2012, at 12:52 AM, Patrik Hägglund H
wrote:<br><br>> Is there anything more I can do to make this patch
accepted?<br>><br>> /Patrik Hägglund<br>><br>> From: Patrik
Hägglund H<br>> Sent: den 26 april 2012 15:33<br>> To: 'Chandler
Carruth'; Bill Wendling<br>> Cc: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>>
Subject: RE: [llvm-commits] [PATCH] optsize attribute on top of -Oz gives
bigger code<br>><br>> Sorry for the late reply.<br>><br>> > 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.<br>><br>> My 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).<br>> /Patrik Hägglund<br>><br>>
From: Chandler Carruth [mailto:<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>]<br>> Sent: den
5 april 2012 00:45<br>> To: Bill Wendling<br>> Cc: Patrik Hägglund H; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>>
Subject: Re: [llvm-commits] [PATCH] optsize attribute on top of -Oz gives
bigger code<br>><br>> On Wed, Apr 4, 2012 at 11:17 PM, Bill Wendling
<<a href="mailto:wendling@apple.com" target="_blank">wendling@apple.com</a>>
wrote:<br>> 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>><br>> I heard that.
;]<br>><br>> 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.<br>><br>> I think there is more work to be done here,
but this is a strict improvement and gives a nice lift, so LGTM!
=D<br>><br>><br>> -bw<br>><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" target="_blank">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" target="_blank">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" target="_blank">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" target="_blank">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>> > <size.diff><br>><br>><br>>
_______________________________________________<br>> llvm-commits mailing
list<br>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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><br></blockquote></div></div></div></div>
</blockquote></div><br>