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

[Bill Wendling]

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. :-)

-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>