<!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><SPAN class=493283807-16052012>> </SPAN>do you have commit access
<DIV dir=ltr align=left><FONT face=Arial color=#0000ff
size=2></FONT> </DIV>
<DIV dir=ltr align=left><SPAN class=493283807-16052012><FONT face=Arial
color=#0000ff size=2>No. Perhaps I should apply for it?</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=493283807-16052012><FONT face=Arial
color=#0000ff size=2></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=493283807-16052012><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 15 maj 2012
16:38<BR><B>To:</B> Patrik Hägglund H<BR><B>Cc:</B>
<llvm-commits@cs.uiuc.edu><BR><B>Subject:</B> Re: [llvm-commits] [PATCH]
opt -Os/-Oz<BR></FONT><BR></DIV>
<DIV></DIV>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="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"><U></U>
<DIV>
<DIV dir=ltr align=left>
<DIV><FONT face=Arial><FONT color=#0000ff><FONT size=+0><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 size=+0><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 size=+0><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></BODY></HTML>