[llvm-commits] [PATCH] opt -Os/-Oz
Evan Cheng
evan.cheng at apple.com
Tue May 15 11:42:20 PDT 2012
While adding opt options is useful, the thing we really need is to model -Oz as a new attribute. Right now we have OptimizeForSize, it really should be split into two: OptimizeForSizeAndSpeed and OptimizeForSize. The former is what OptimizeForSize currently is (for -Os), the later should be for -Oz.
Evan
On May 15, 2012, at 7:38 AM, Chandler Carruth wrote:
> LGTM, do you have commit access? if not I'll commit shortly.
>
> On Tue, May 15, 2012 at 5:59 AM, Patrik Hägglund H <patrik.h.hagglund at ericsson.com> wrote:
> > 1) Can you separate out the opt tool change and commit that separately? I think that makes for a cleaner set of changes.
>
> OK.
>
> > This... doesn't seem right. For O1, O2, and O3, I think the idea is that each is a superset of the previous, and so adding them in sequence makes sense.... but I thought that Os and Oz actually ran fewer optimizations passes than O2, an the certainly run fewer than O3 don't they?
>
> The -Os and -Oz options currently use the same passes as -O2, and as said below, -O passes are not strict supersets of lower levels.
>
> Therefore, I just reordered the OptLevel tests (O1, O2, Os, Oz, O3, instead of the previous order O1, O2, O3, Os, Oz). The order matters only if multiple -O options are specified, which I think is dubious to expect to have any meaning.
>
> > 2) It would be nice to get a very boring test case that just tries to use -Os and -Oz in the 'opt' tool. Ideally, we would ask the opt tool to dump the pass manager information and FileCheck over it.
>
> I'm not sure what you want to check for. Passes for each of the -O options could be unrelated. In practice they relate, but not in any way I can imagine that has to be _enforced_. I included a test that just check that -debug-pass=Arguments outputs something for each of the -O options.
>
> /Patrik Hägglund
>
> From: Patrik Hägglund H
> Sent: den 14 maj 2012 16:28
> To: 'Chandler Carruth'
> Cc: <llvm-commits at cs.uiuc.edu>
> Subject: RE: [llvm-commits] [PATCH] optsize attribute on top of -Oz gives bigger code
>
> > I think the idea is that each is a superset of the previous
>
> Hmm... but they are not. For example, -always-inline in 'opt -O1' is removed in 'opt -O2' (replaced by -inline). I assumed that the result of using multiple -O options was undefined.
>
> I look at this more tomorrow.
>
> /Patrik Hägglund
>
> From: Chandler Carruth [mailto:chandlerc at google.com]
> Sent: den 12 maj 2012 04:08
> To: Patrik Hägglund H
> Cc: Bill Wendling; <llvm-commits at cs.uiuc.edu>
> Subject: Re: [llvm-commits] [PATCH] optsize attribute on top of -Oz gives bigger code
>
> On Thu, May 10, 2012 at 7:47 AM, Patrik Hägglund H <patrik.h.hagglund at ericsson.com> wrote:
> OK. I checked this a second time, and found that it wasn't that hard to add support for opt -Os/-Oz, to mirror the clang driver.
>
> The attached patch contains
> * my previous patch for the inliner,
> * the changes to the opt driver, and
> * a testcase to test the inliner.
>
> The opt -Os and -Oz changes has been tested to compile and execute our own (quite small) collection of C programs.
>
> Very cool, the test case is nice. =]
>
> A couple of requests:
>
> 1) Can you separate out the opt tool change and commit that separately? I think that makes for a cleaner set of changes.
> 2) It would be nice to get a very boring test case that just tries to use -Os and -Oz in the 'opt' tool. Ideally, we would ask the opt tool to dump the pass manager information and FileCheck over it.
>
> Also, some comments on the patches themselves:
>
> diff --git a/tools/opt/opt.cpp b/tools/opt/opt.cpp
> index a5b0511..c879d4b 100644
> --- a/tools/opt/opt.cpp
> +++ b/tools/opt/opt.cpp
> @@ -104,15 +104,23 @@ StandardLinkOpts("std-link-opts",
>
> static cl::opt<bool>
> OptLevelO1("O1",
> - cl::desc("Optimization level 1. Similar to llvm-gcc -O1"));
> + cl::desc("Optimization level 1. Similar to clang -O1"));
>
> static cl::opt<bool>
> OptLevelO2("O2",
> - cl::desc("Optimization level 2. Similar to llvm-gcc -O2"));
> + cl::desc("Optimization level 2. Similar to clang -O2"));
>
> static cl::opt<bool>
> OptLevelO3("O3",
> - cl::desc("Optimization level 3. Similar to llvm-gcc -O3"));
> + cl::desc("Optimization level 3. Similar to clang -O3"));
> +
> +static cl::opt<bool>
> +OptLevelOs("Os",
> + cl::desc("Like -O2 with extra optimizations for size. Similar to clang -Os"));
> +
> +static cl::opt<bool>
> +OptLevelOz("Oz",
> + cl::desc("Like -Os but reduces code size further. Similar to clang -Oz"));
>
> static cl::opt<std::string>
> TargetTriple("mtriple", cl::desc("Override target triple for module"));
> @@ -409,16 +417,21 @@ static inline void addPass(PassManagerBase &PM, Pass *P) {
> ///
> /// OptLevel - Optimization Level
> static void AddOptimizationPasses(PassManagerBase &MPM,FunctionPassManager &FPM,
> - unsigned OptLevel) {
> + unsigned OptLevel, unsigned SizeLevel) {
> FPM.add(createVerifierPass()); // Verify that input is correct
>
> PassManagerBuilder Builder;
> Builder.OptLevel = OptLevel;
> + Builder.SizeLevel = SizeLevel;
>
> if (DisableInline) {
> // No inlining pass
> } else if (OptLevel > 1) {
> unsigned Threshold = 225;
> + if (SizeLevel == 1) // -Os
> + Threshold = 75;
> + else if (SizeLevel == 2) // -Oz
> + Threshold = 25;
> if (OptLevel > 2)
> Threshold = 275;
> Builder.Inliner = createFunctionInliningPass(Threshold);
> @@ -571,7 +584,7 @@ int main(int argc, char **argv) {
> Passes.add(TD);
>
> OwningPtr<FunctionPassManager> FPasses;
> - if (OptLevelO1 || OptLevelO2 || OptLevelO3) {
> + if (OptLevelO1 || OptLevelO2 || OptLevelO3 || OptLevelOs || OptLevelOz) {
> FPasses.reset(new FunctionPassManager(M.get()));
> if (TD)
> FPasses->add(new TargetData(*TD));
> @@ -617,20 +630,30 @@ int main(int argc, char **argv) {
> }
>
> if (OptLevelO1 && OptLevelO1.getPosition() < PassList.getPosition(i)) {
> - AddOptimizationPasses(Passes, *FPasses, 1);
> + AddOptimizationPasses(Passes, *FPasses, 1, 0);
> OptLevelO1 = false;
> }
>
> if (OptLevelO2 && OptLevelO2.getPosition() < PassList.getPosition(i)) {
> - AddOptimizationPasses(Passes, *FPasses, 2);
> + AddOptimizationPasses(Passes, *FPasses, 2, 0);
> OptLevelO2 = false;
> }
>
> if (OptLevelO3 && OptLevelO3.getPosition() < PassList.getPosition(i)) {
> - AddOptimizationPasses(Passes, *FPasses, 3);
> + AddOptimizationPasses(Passes, *FPasses, 3, 0);
> OptLevelO3 = false;
> }
>
> + if (OptLevelOs && OptLevelOs.getPosition() < PassList.getPosition(i)) {
> + AddOptimizationPasses(Passes, *FPasses, 2, 1);
> + OptLevelOs = false;
> + }
> +
> + if (OptLevelOz && OptLevelOz.getPosition() < PassList.getPosition(i)) {
> + AddOptimizationPasses(Passes, *FPasses, 2, 2);
> + OptLevelOz = false;
> + }
> +
> const PassInfo *PassInf = PassList[i];
> Pass *P = 0;
> if (PassInf->getNormalCtor())
> @@ -682,15 +705,21 @@ int main(int argc, char **argv) {
> }
>
> if (OptLevelO1)
> - AddOptimizationPasses(Passes, *FPasses, 1);
> + AddOptimizationPasses(Passes, *FPasses, 1, 0);
>
> if (OptLevelO2)
> - AddOptimizationPasses(Passes, *FPasses, 2);
> + AddOptimizationPasses(Passes, *FPasses, 2, 0);
>
> if (OptLevelO3)
> - AddOptimizationPasses(Passes, *FPasses, 3);
> + AddOptimizationPasses(Passes, *FPasses, 3, 0);
> +
> + if (OptLevelOs)
> + AddOptimizationPasses(Passes, *FPasses, 2, 1);
> +
> + if (OptLevelOz)
> + AddOptimizationPasses(Passes, *FPasses, 2, 2);
>
> This... doesn't seem right. For O1, O2, and O3, I think the idea is that each is a superset of the previous, and so adding them in sequence makes sense.... but I thought that Os and Oz actually ran fewer optimizations passes than O2, an the certainly run fewer than O3 don't they?
>
> I wonder if this could be caught with the test I mentioned? Not sure, it might be hard to interpret the tea leaves of the opt pass debugging output.
>
>
> On the original patch to fix things, a few very minor nits on the test case. I think this one is essentially good to go in once these are addressed (and the opt patch landed naturally):
>
> diff --git a/test/Transforms/Inline/inline-optsize.ll b/test/Transforms/Inline/inline-optsize.ll
> new file mode 100644
> index 0000000..1455ea2
> --- /dev/null
> +++ b/test/Transforms/Inline/inline-optsize.ll
> @@ -0,0 +1,35 @@
> +; RUN: opt -S -Oz %s | FileCheck %s
> +; xxx: clang -cc1 -S -emit-llvm -o - -Oz %s | FileCheck %s
>
> No need for the clang line here...
>
> +
> +; The inline threshold for a function with the optsize attribute is currently
> +; the same as the global inline threshold for -Os. Check that the optsize
> +; function attribute don't alter the function specific inline threshold if the
> +; global inline threshold is lower (as for -Oz).
> +
> + at a = global i32 4
> +
> +; This function should be larger than the inline threshold for -Oz (25), but
> +; smaller than the inline threshold for optsize (75). It should not be inlined
> +; below.
>
> It's important to make sure that we don't regress in the other direction here -- we might decide to never inline this function regardless of the threshold, and then this test might pass for the wrong reason.
>
> My suggestion would be to run opt and filecheck twice. Once just as you have it, and once with a different --check-prefix and -O2 or something to assert that it does get inlined in that case.
>
> Thanks!
> -Chandler
>
> +define i32 @inner() {
> + %a1 = load volatile i32* @a
> + %x1 = add i32 %a1, %a1
> + %a2 = load volatile i32* @a
> + %x2 = add i32 %x1, %a2
> + %a3 = load volatile i32* @a
> + %x3 = add i32 %x2, %a3
> + %a4 = load volatile i32* @a
> + %x4 = add i32 %x3, %a4
> + %a5 = load volatile i32* @a
> + %x5 = add i32 %x3, %a5
> + ret i32 %x5
> +}
> +
> +define i32 @outer() optsize {
> +; CHECK: @outer
> +; CHECK: call
> +; CHECK: ret
> +
> + %r = call i32 @inner()
> + ret i32 %r
> +}
>
>
> _______________________________________________
> 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/20120515/ff068e20/attachment.html>
More information about the llvm-commits
mailing list