[llvm-commits] [PATCH] optsize attribute on top of -Oz gives bigger code

Chandler Carruth chandlerc at google.com
Tue May 8 00:19:55 PDT 2012 

On Mon, May 7, 2012 at 6:34 AM, Patrik Hägglund H <
patrik.h.hagglund at ericsson.com> wrote:

> **
> Hi,
>
> Any progress?
>

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.


>
> I wrote:
> > I can't see that any tool  in the llvm tree (specifically opt) is
> invoking that constructor.
>
> Can you confirm or refute this?
>

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.

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.

-Chandler


>
> /Patrik Hägglund
>
>  ------------------------------
> *From:* Chandler Carruth [mailto:chandlerc at google.com]
> *Sent:* den 30 april 2012 12:21
>
> *To:* Bill Wendling
> *Cc:* Patrik Hägglund H; <llvm-commits at cs.uiuc.edu>
> *Subject:* Re: [llvm-commits] [PATCH] optsize attribute on top of -Oz
> gives bigger code
>
>  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.
> On Apr 30, 2012 3:19 AM, "Bill Wendling" <wendling at apple.com> wrote:
>
>> Sorry. I think if you can cobble together some test cases, then it would
>> be good.
>>
>> -bw
>>
>> On Apr 30, 2012, at 12:52 AM, Patrik Hägglund H wrote:
>>
>> > Is there anything more I can do to make this patch accepted?
>> >
>> > /Patrik Hägglund
>> >
>> > From: Patrik Hägglund H
>> > Sent: den 26 april 2012 15:33
>> > To: 'Chandler Carruth'; Bill Wendling
>> > Cc: llvm-commits at cs.uiuc.edu
>> > Subject: RE: [llvm-commits] [PATCH] optsize attribute on top of -Oz
>> gives bigger code
>> >
>> > Sorry for the late reply.
>> >
>> > > 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.
>> >
>> > 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).
>> > /Patrik Hägglund
>> >
>> > From: Chandler Carruth [mailto:chandlerc at google.com]
>> > Sent: den 5 april 2012 00:45
>> > To: Bill Wendling
>> > Cc: Patrik Hägglund H; llvm-commits at cs.uiuc.edu
>> > Subject: Re: [llvm-commits] [PATCH] optsize attribute on top of -Oz
>> gives bigger code
>> >
>> > On Wed, Apr 4, 2012 at 11:17 PM, Bill Wendling <wendling at apple.com>
>> wrote:
>> > Hi Patrik,
>> >
>> > 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. :-)
>> >
>> > I heard that. ;]
>> >
>> > 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.
>> >
>> > I think there is more work to be done here, but this is a strict
>> improvement and gives a nice lift, so LGTM! =D
>> >
>> >
>> > -bw
>> >
>> > On Apr 4, 2012, at 6:14 AM, Patrik Hägglund H wrote:
>> >
>> > > Hi Bill,
>> > >
>> > > 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.
>> > >
>> > > 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.
>> > >
>> > > 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).
>> > >
>> > > Here is the updated patch:
>> > >
>> > > diff --git a/lib/Transforms/IPO/Inliner.cpp
>> b/lib/Transforms/IPO/Inliner.cpp
>> > > index 8a9d149..4e13596 100644
>> > > --- a/lib/Transforms/IPO/Inliner.cpp
>> > > +++ b/lib/Transforms/IPO/Inliner.cpp
>> > > @@ -196,19 +196,23 @@ static bool InlineCallIfPossible(CallSite CS,
>> InlineFunctionInfo &IFI,
>> > > }
>> > >
>> > > unsigned Inliner::getInlineThreshold(CallSite CS) const {
>> > > -  int thres = InlineThreshold;
>> > > +  int thres = InlineThreshold; // -inline-threshold or else selected
>> by
>> > > +                               // overall opt level
>> > >
>> > > -  // Listen to optsize when -inline-limit is not given.
>> > > +  // If -inline-threshold is not given, listen to the optsize
>> attribute when it
>> > > +  // would decrease the threshold.
>> > >   Function *Caller = CS.getCaller();
>> > > -  if (Caller && !Caller->isDeclaration() &&
>> > > -      Caller->hasFnAttr(Attribute::OptimizeForSize) &&
>> > > +  bool OptSize = Caller && !Caller->isDeclaration() &&
>> > > +    Caller->hasFnAttr(Attribute::OptimizeForSize);
>> > > +  if (OptSize && OptSizeThreshold < thres &&
>> > >       InlineLimit.getNumOccurrences() == 0)
>> > >     thres = OptSizeThreshold;
>> > >
>> > > -  // Listen to inlinehint when it would increase the threshold.
>> > > +  // Listen to the inlinehint attribute when it would increase the
>> threshold.
>> > >   Function *Callee = CS.getCalledFunction();
>> > > -  if (HintThreshold > thres && Callee && !Callee->isDeclaration() &&
>> > > -      Callee->hasFnAttr(Attribute::InlineHint))
>> > > +  bool InlineHint = Callee && !Callee->isDeclaration() &&
>> > > +    Callee->hasFnAttr(Attribute::InlineHint);
>> > > +  if (InlineHint && HintThreshold > thres)
>> > >     thres = HintThreshold;
>> > >
>> > >   return thres;
>> > >
>> > > /Patrik Hägglund
>> > >
>> > > -----Original Message-----
>> > > From: Patrik Hägglund H
>> > > Sent: den 22 mars 2012 08:57
>> > > To: 'Bill Wendling'
>> > > Cc: llvm-commits at cs.uiuc.edu
>> > > Subject: RE: [llvm-commits] [PATCH] optsize attribute on top of -Oz
>> gives bigger code
>> > >
>> > >> Could you give some metrics to go along with the patch? You should
>> be able to measure the impact on the LLVM test suite.
>> > >
>> > > 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.
>> > >
>> > > 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)?
>> > >
>> > >> A comment on the patch itself:
>> > >
>> > > 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.
>> > >
>> > > /Patrik Hägglund
>> > >
>> > > -----Original Message-----
>> > > From: Bill Wendling [mailto:wendling at apple.com]
>> > > Sent: den 22 mars 2012 08:10
>> > > To: Patrik Hägglund H
>> > > Cc: llvm-commits at cs.uiuc.edu
>> > > Subject: Re: [llvm-commits] [PATCH] optsize attribute on top of -Oz
>> gives bigger code
>> > >
>> > > Hi Patrik,
>> > >
>> > > 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.
>> > >
>> > > A comment on the patch itself:
>> > >
>> > > +  if (!(InlineLimit.getNumOccurrences() > 0) && OptSize &&
>> OptSizeThreshold < thres)
>> > >
>> > > The logic could be made better (and more readable). Please use this
>> instead:
>> > >
>> > > +  if (OptSize && OptSizeThreshold < thres &&
>> InlineLimit.getNumOccurrences() == 0)
>> > >
>> > > -bw
>> > >
>> > > On Mar 20, 2012, at 6:30 AM, Patrik Hägglund H wrote:
>> > >
>> > >> Hi,
>> > >>
>> > >> 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.
>> > >>
>> > >> The base inline threshold was 25 with clang -Oz, but 75 when adding
>> the optsize attribute.
>> > >>
>> > >> This patch fix this to only change the inline threshold for a
>> function with optsize if it lowers the base threshold.
>> > >>
>> > >> /Patrik Hägglund
>> > >>
>> > >> <optsize_inliner.diff>_______________________________________________
>> > >> llvm-commits mailing list
>> > >> llvm-commits at cs.uiuc.edu
>> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> > >
>> > > <size.diff>
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120508/bbb4441b/attachment.html>

More information about the llvm-commits mailing list