[PATCH] D20576: [Driver] Add support for -finline-functions and /Ob2 flags

[Hans Wennborg via cfe-commits]

hans added a comment.

Thanks! Some comments below:


================
Comment at: lib/Driver/Tools.cpp:5335
@@ -5334,3 +5334,3 @@
 
-  if (Args.hasArg(options::OPT_fno_inline_functions))
-    CmdArgs.push_back("-fno-inline-functions");
+  if (Arg* InlineArg = Args.getLastArg(options::OPT_finline_functions, options::OPT_fno_inline_functions)) {
+    CmdArgs.push_back(InlineArg->getOption().matches(options::OPT_finline_functions) ?
----------------
Please line break this; lines should be <= 80 columns wide.

================
Comment at: lib/Driver/Tools.cpp:5337
@@ +5336,3 @@
+    CmdArgs.push_back(InlineArg->getOption().matches(options::OPT_finline_functions) ?
+        "-finline-functions" : "-fno-inline-functions");
+  }
----------------
I think this can be:

```
InlineArg->render(Args, CmdArgs)
```
instead

================
Comment at: lib/Frontend/CompilerInvocation.cpp:446
@@ +445,3 @@
+    Opts.setInlining(InlineArg->getOption().matches(options::OPT_finline_functions) ?
+        CodeGenOptions::NormalInlining : CodeGenOptions::OnlyAlwaysInlining);
+  }
----------------
This line and 444 need line breaks too.

================
Comment at: test/CodeGen/inline-optim.c:16
@@ +15,3 @@
+// NOINLINE: @foo
+// INLINE: @foo
+// NOINLINE: inline_hint
----------------
I'd suggest using "check-label" for the @foo checks. See http://llvm.org/docs/CommandGuide/FileCheck.html#the-check-label-directive

================
Comment at: test/CodeGen/inline-optim.c:24
@@ +23,3 @@
+// NOINLINE: inline_no_hint
+// INLINE-NOT: inline_no_hint
+    pa[6] = inline_no_hint(pa[7], pa[8]);
----------------
For all these checks, I'd suggest matching e.g.

 "call i32 @inline_hint"

to make it a little easier to see what' being matched for. I suppose your checks could match either the calls, or the definitions of the called function.


http://reviews.llvm.org/D20576