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

[Chandler Carruth]

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/20120405/63c0c73e/attachment.html>