[PATCH] D28053: Cleanup the handling of noinline function attributes, -fno-inline, -fno-inline-functions, -O0, and optnone.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:453
+    // There is no effect at O0 when we won't inline anyways.
+    if (Opts.OptimizationLevel > 1) {
+      const Option &InlineOpt = InlineArg->getOption();
----------------
chandlerc wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > I'd switch the two if
> > The test `> 1` and the comment about `-O0` are confusing to me as well. What about the following (IIUC):
> > 
> > ```
> > if (Opts.OptimizationLevel <= 1) {
> >   // "always-inline" required for correctness at O0 and O1.
> >   Opts.setInlining(CodeGenOptions::OnlyAlwaysInlining);
> > else if (Opts.OptimizationLevel > 1) {
> >   // Normal inlining at O2 and above
> >   Opts.setInlining((CodeGenOptions::NormalInlining);
> >   // -fno-inline-functions overrides
> >   if (Arg *InlineArg = Args.getLastArg(
> >           options::OPT_finline_functions, options::OPT_finline_hint_functions,
> >           options::OPT_fno_inline_functions, options::OPT_fno_inline)) {
> >      if (InlineOpt.matches(options::OPT_finline_functions))
> >         Opts.setInlining(CodeGenOptions::NormalInlining);
> >       else if (InlineOpt.matches(options::OPT_finline_hint_functions))
> >         Opts.setInlining(CodeGenOptions::OnlyHintInlining);
> >       else
> >         Opts.setInlining(CodeGenOptions::OnlyAlwaysInlining);
> >   }
> > }
> > ```
> > 
> Yea, this was confusing, and I hadn't actually implemented my intent here either.
> 
> My goal was to make the `Inlining` setting correspond to whether we *forcibly* disable some aspects of inlining or not, rather than anything to do with what inliner pass we run.
> 
> This means we should only be setting `OnlyAlwaysInlining` in O0, not at O1, and the O1 behavior should be up to the backend's construction of a pass pipeline appropriate to that optimization level. (I actually think using the always inliner is a mistake here, but that's a separate discussion.)
> 
> To make all of this work requires me to merge in the next patch I was working on which I've done. It doesn't really scope creep much.
> 
> I've updated the formulation of the conditions and the comments to hopefully be more clear.
> 
> I'm still doing the weird order of testing because i want to always run `getLastArg` to mark these flags as used.
> always run getLastArg to mark these flags as used.

Oh I missed this side effect in the test, that's nasty. Is it covered by a test? (i.e. if someone refactor this code in the future will we warn for unused and break a test?).


https://reviews.llvm.org/D28053