LGTM, do you have commit access? if not I'll commit shortly.<br><br><div class="gmail_quote">On Tue, May 15, 2012 at 5:59 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">
<div><font face="Arial"><font color="#0000ff"><font><span>> </span>1) Can you separate out the opt tool change
and commit that separately? I think that makes for a cleaner set of
changes.</font></font></font></div>
<div><font face="Arial" color="#0000ff"></font> </div>
<div><span><font face="Arial" color="#0000ff">OK.</font></span></div>
<div><span><font face="Arial" color="#0000ff"></font></span> </div>
<div><span>
<div><font face="Arial"><font color="#0000ff"><font><span>> </span>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?</font></font></font></div></span></div>
<div><span><font face="Arial" color="#0000ff"></font></span> </div>
<div><span><font face="Arial" color="#0000ff">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.</font></span></div>
<div><span><font face="Arial" color="#0000ff"></font></span> </div>
<div><span><font face="Arial" color="#0000ff">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.</font></span></div></div>
<div><font face="Arial" color="#0000ff"></font> </div>
<div><span>
<div><font face="Arial"><font color="#0000ff"><font><span>> </span>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.</font></font></font></div>
<div><font face="Arial" color="#0000ff"></font> </div>
<div><span><font face="Arial" color="#0000ff">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.</font></span></div></span></div>
<div><font face="Arial" color="#0000ff"></font><font face="Arial" color="#0000ff"></font><br><span><font face="Arial" color="#0000ff">/Patrik Hägglund</font></span></div>
<div><span><font face="Arial" color="#0000ff"></font></span> </div>
<div lang="en-us" dir="ltr" align="left">
<hr>
<font face="Tahoma"><b>From:</b> Patrik Hägglund H <br><b>Sent:</b> den 14
maj 2012 16:28<br><b>To:</b> 'Chandler Carruth'<br><b>Cc:</b>
<<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>><br><b>Subject:</b> RE: [llvm-commits] [PATCH]
optsize attribute on top of -Oz gives bigger code<br></font><br></div>
<div></div>
<div dir="ltr" align="left"><span>> </span>I think the
idea is that each is a superset of the previous</div>
<div dir="ltr" align="left"><font face="Arial" color="#0000ff"></font> </div>
<div dir="ltr" align="left"><span><font face="Arial" color="#0000ff">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.</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">I look at this more tomorrow.</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">/Patrik Hägglund</font></span></div><br>
<div lang="en-us" dir="ltr" align="left">
<hr>
<font face="Tahoma"><b>From:</b> Chandler Carruth
[mailto:<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>] <br><b>Sent:</b> den 12 maj 2012
04:08<br><b>To:</b> Patrik Hägglund H<br><b>Cc:</b> Bill Wendling;
<<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>><br><b>Subject:</b> Re: [llvm-commits] [PATCH]
optsize attribute on top of -Oz gives bigger code<br></font><br></div>
<div></div>
<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="PADDING-LEFT:1ex;MARGIN:0px 0px 0px 0.8ex;BORDER-LEFT:#ccc 1px solid"><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></div>
</blockquote></div><br>