[llvm-commits] [PATCH] optsize attribute on top of -Oz gives bigger code
Chandler Carruth
chandlerc at google.com
Mon Apr 30 03:21:08 PDT 2012
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/20120430/566e9739/attachment.html>
More information about the llvm-commits
mailing list