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

Mon May 7 06:34:58 PDT 2012

Hi,

Any progress?

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?

/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<mailto: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<mailto: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<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<mailto: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<mailto: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<mailto: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<mailto:wendling at apple.com>]
> > Sent: den 22 mars 2012 08:10
> > To: Patrik Hägglund H
> > Cc: llvm-commits at cs.uiuc.edu<mailto: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<mailto: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<mailto: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/20120507/0ad5b029/attachment.html>