<div class="gmail_quote">On Thu, May 10, 2012 at 7:47 AM, Patrik Hägglund H <span dir="ltr"><<a href="mailto:patrik.h.hagglund@ericsson.com" target="_blank">patrik.h.hagglund@ericsson.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<u></u>
<div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">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.</font></span></div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">The attached patch contains</font></span></div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">* my previous patch for the
inliner,</font></span></div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">* the changes to the opt driver,
and</font></span></div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">* a testcase to test the inliner.</font></span></div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">The opt -Os and -Oz changes has been tested to compile
and execute our own (quite small) collection of C
programs.</font></span></div></div></blockquote><div><br></div><div>Very cool, the test case is nice. =]</div><div><br></div><div>A couple of requests:</div><div><br></div><div>1) Can you separate out the opt tool change and commit that separately? I think that makes for a cleaner set of changes.</div>
<div>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.</div><div>
<br></div><div>Also, some comments on the patches themselves:</div><div><br></div><div><div>diff --git a/tools/opt/opt.cpp b/tools/opt/opt.cpp</div><div>index a5b0511..c879d4b 100644</div><div>--- a/tools/opt/opt.cpp</div>
<div>+++ b/tools/opt/opt.cpp</div><div>@@ -104,15 +104,23 @@ StandardLinkOpts("std-link-opts",</div><div><br></div><div> static cl::opt<bool></div><div> OptLevelO1("O1",</div><div>- cl::desc("Optimization level 1. Similar to llvm-gcc -O1"));</div>
<div>+ cl::desc("Optimization level 1. Similar to clang -O1"));</div><div><br></div><div> static cl::opt<bool></div><div> OptLevelO2("O2",</div><div>- cl::desc("Optimization level 2. Similar to llvm-gcc -O2"));</div>
<div>+ cl::desc("Optimization level 2. Similar to clang -O2"));</div><div><br></div><div> static cl::opt<bool></div><div> OptLevelO3("O3",</div><div>- cl::desc("Optimization level 3. Similar to llvm-gcc -O3"));</div>
<div>+ cl::desc("Optimization level 3. Similar to clang -O3"));</div><div>+</div><div>+static cl::opt<bool></div><div>+OptLevelOs("Os",</div><div>+ cl::desc("Like -O2 with extra optimizations for size. Similar to clang -Os"));</div>
<div>+</div><div>+static cl::opt<bool></div><div>+OptLevelOz("Oz",</div><div>+ cl::desc("Like -Os but reduces code size further. Similar to clang -Oz"));</div><div><br></div><div> static cl::opt<std::string></div>
<div> TargetTriple("mtriple", cl::desc("Override target triple for module"));</div><div>@@ -409,16 +417,21 @@ static inline void addPass(PassManagerBase &PM, Pass *P) {</div><div> ///</div><div> /// OptLevel - Optimization Level</div>
<div> static void AddOptimizationPasses(PassManagerBase &MPM,FunctionPassManager &FPM,</div><div>- unsigned OptLevel) {</div><div>+ unsigned OptLevel, unsigned SizeLevel) {</div>
<div> FPM.add(createVerifierPass()); // Verify that input is correct</div><div><br></div><div> PassManagerBuilder Builder;</div><div> Builder.OptLevel = OptLevel;</div><div>+ Builder.SizeLevel = SizeLevel;</div>
<div><br></div><div> if (DisableInline) {</div><div> // No inlining pass</div><div> } else if (OptLevel > 1) {</div><div> unsigned Threshold = 225;</div><div>+ if (SizeLevel == 1) // -Os</div><div>+ Threshold = 75;</div>
<div>+ else if (SizeLevel == 2) // -Oz</div><div>+ Threshold = 25;</div><div> if (OptLevel > 2)</div><div> Threshold = 275;</div><div> Builder.Inliner = createFunctionInliningPass(Threshold);</div>
<div>@@ -571,7 +584,7 @@ int main(int argc, char **argv) {</div><div> Passes.add(TD);</div><div><br></div><div> OwningPtr<FunctionPassManager> FPasses;</div><div>- if (OptLevelO1 || OptLevelO2 || OptLevelO3) {</div>
<div>+ if (OptLevelO1 || OptLevelO2 || OptLevelO3 || OptLevelOs || OptLevelOz) {</div><div> FPasses.reset(new FunctionPassManager(M.get()));</div><div> if (TD)</div><div> FPasses->add(new TargetData(*TD));</div>
<div>@@ -617,20 +630,30 @@ int main(int argc, char **argv) {</div><div> }</div><div><br></div><div> if (OptLevelO1 && OptLevelO1.getPosition() < PassList.getPosition(i)) {</div><div>- AddOptimizationPasses(Passes, *FPasses, 1);</div>
<div>+ AddOptimizationPasses(Passes, *FPasses, 1, 0);</div><div> OptLevelO1 = false;</div><div> }</div><div><br></div><div> if (OptLevelO2 && OptLevelO2.getPosition() < PassList.getPosition(i)) {</div>
<div>- AddOptimizationPasses(Passes, *FPasses, 2);</div><div>+ AddOptimizationPasses(Passes, *FPasses, 2, 0);</div><div> OptLevelO2 = false;</div><div> }</div><div><br></div><div> if (OptLevelO3 && OptLevelO3.getPosition() < PassList.getPosition(i)) {</div>
<div>- AddOptimizationPasses(Passes, *FPasses, 3);</div><div>+ AddOptimizationPasses(Passes, *FPasses, 3, 0);</div><div> OptLevelO3 = false;</div><div> }</div><div><br></div><div>+ if (OptLevelOs && OptLevelOs.getPosition() < PassList.getPosition(i)) {</div>
<div>+ AddOptimizationPasses(Passes, *FPasses, 2, 1);</div><div>+ OptLevelOs = false;</div><div>+ }</div><div>+</div><div>+ if (OptLevelOz && OptLevelOz.getPosition() < PassList.getPosition(i)) {</div>
<div>+ AddOptimizationPasses(Passes, *FPasses, 2, 2);</div><div>+ OptLevelOz = false;</div><div>+ }</div><div>+</div><div> const PassInfo *PassInf = PassList[i];</div><div> Pass *P = 0;</div><div> if (PassInf->getNormalCtor())</div>
<div>@@ -682,15 +705,21 @@ int main(int argc, char **argv) {</div><div> }</div><div><br></div><div> if (OptLevelO1)</div><div>- AddOptimizationPasses(Passes, *FPasses, 1);</div><div>+ AddOptimizationPasses(Passes, *FPasses, 1, 0);</div>
<div><br></div><div> if (OptLevelO2)</div><div>- AddOptimizationPasses(Passes, *FPasses, 2);</div><div>+ AddOptimizationPasses(Passes, *FPasses, 2, 0);</div><div><br></div><div> if (OptLevelO3)</div><div>- AddOptimizationPasses(Passes, *FPasses, 3);</div>
<div>+ AddOptimizationPasses(Passes, *FPasses, 3, 0);</div><div>+</div><div>+ if (OptLevelOs)</div><div>+ AddOptimizationPasses(Passes, *FPasses, 2, 1);</div><div>+</div><div>+ if (OptLevelOz)</div><div>+ AddOptimizationPasses(Passes, *FPasses, 2, 2);</div>
<div><br></div><div>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?</div>
<div><br></div><div>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.</div><div><br></div><div><br></div></div><div>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):</div>
<div><br></div><div><div>diff --git a/test/Transforms/Inline/inline-optsize.ll b/test/Transforms/Inline/inline-optsize.ll</div><div>new file mode 100644</div><div>index 0000000..1455ea2</div><div>--- /dev/null</div><div>+++ b/test/Transforms/Inline/inline-optsize.ll</div>
<div>@@ -0,0 +1,35 @@</div><div>+; RUN: opt -S -Oz %s | FileCheck %s</div><div>+; xxx: clang -cc1 -S -emit-llvm -o - -Oz %s | FileCheck %s</div><div><br></div><div>No need for the clang line here...</div><div><br></div><div>
+</div><div>+; The inline threshold for a function with the optsize attribute is currently</div><div>+; the same as the global inline threshold for -Os. Check that the optsize</div><div>+; function attribute don't alter the function specific inline threshold if the</div>
<div>+; global inline threshold is lower (as for -Oz).</div><div>+</div><div>+@a = global i32 4</div><div>+</div><div>+; This function should be larger than the inline threshold for -Oz (25), but</div><div>+; smaller than the inline threshold for optsize (75). It should not be inlined</div>
<div>+; below.</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div><div>
<br></div><div>Thanks!</div><div>-Chandler</div><div><br></div><div>+define i32 @inner() {</div><div>+ %a1 = load volatile i32* @a</div><div>+ %x1 = add i32 %a1, %a1</div><div>+ %a2 = load volatile i32* @a</div><div>+ %x2 = add i32 %x1, %a2</div>
<div>+ %a3 = load volatile i32* @a</div><div>+ %x3 = add i32 %x2, %a3</div><div>+ %a4 = load volatile i32* @a</div><div>+ %x4 = add i32 %x3, %a4</div><div>+ %a5 = load volatile i32* @a</div><div>+ %x5 = add i32 %x3, %a5</div>
<div>+ ret i32 %x5</div><div>+}</div><div>+</div><div>+define i32 @outer() optsize {</div><div>+; CHECK: @outer</div><div>+; CHECK: call</div><div>+; CHECK: ret</div><div>+</div><div>+ %r = call i32 @inner()</div><div>
+ ret i32 %r</div><div>+}</div></div><div><br></div></div>