<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META http-equiv=Content-Type content="text/html; charset=iso-8859-1">
<META content="MSHTML 6.00.6002.18591" name=GENERATOR></HEAD>
<BODY>
<DIV dir=ltr align=left>
<DIV><FONT face=Arial><FONT color=#0000ff><FONT size=2><SPAN
class=118582511-15052012>> </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 size=2></FONT> </DIV>
<DIV><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff
size=2>OK.</FONT></SPAN></DIV>
<DIV><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff
size=2></FONT></SPAN> </DIV>
<DIV><SPAN class=118582511-15052012>
<DIV><FONT face=Arial><FONT color=#0000ff><FONT size=2><SPAN
class=118582511-15052012>> </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 class=118582511-15052012><FONT face=Arial color=#0000ff
size=2></FONT></SPAN> </DIV>
<DIV><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff size=2>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 class=118582511-15052012><FONT face=Arial color=#0000ff
size=2></FONT></SPAN> </DIV>
<DIV><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff
size=2>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 size=2></FONT> </DIV>
<DIV><SPAN class=118582511-15052012>
<DIV><FONT face=Arial><FONT color=#0000ff><FONT size=2><SPAN
class=118582511-15052012>> </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 size=2></FONT> </DIV>
<DIV><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff size=2>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 size=2></FONT><FONT face=Arial color=#0000ff
size=2></FONT><BR><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff
size=2>/Patrik Hägglund</FONT></SPAN></DIV>
<DIV><SPAN class=118582511-15052012><FONT face=Arial color=#0000ff
size=2></FONT></SPAN> </DIV>
<DIV class=OutlookMessageHeader lang=en-us dir=ltr align=left>
<HR tabIndex=-1>
<FONT face=Tahoma size=2><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>
<llvm-commits@cs.uiuc.edu><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 class=825502114-14052012>> </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
size=2></FONT> </DIV>
<DIV dir=ltr align=left><SPAN class=825502114-14052012><FONT face=Arial
color=#0000ff size=2>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 class=825502114-14052012><FONT face=Arial
color=#0000ff size=2></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=825502114-14052012><FONT face=Arial
color=#0000ff size=2>I look at this more tomorrow.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=825502114-14052012><FONT face=Arial
color=#0000ff size=2></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=825502114-14052012><FONT face=Arial
color=#0000ff size=2>/Patrik Hägglund</FONT></SPAN></DIV><BR>
<DIV class=OutlookMessageHeader lang=en-us dir=ltr align=left>
<HR tabIndex=-1>
<FONT face=Tahoma size=2><B>From:</B> Chandler Carruth
[mailto:chandlerc@google.com] <BR><B>Sent:</B> den 12 maj 2012
04:08<BR><B>To:</B> Patrik Hägglund H<BR><B>Cc:</B> Bill Wendling;
<llvm-commits@cs.uiuc.edu><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></BODY></HTML>