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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D28053#629834, @mehdi_amini wrote:

> In https://reviews.llvm.org/D28053#629768, @rnk wrote:
>
> > The big change here is that `clang -O0` now applies the noinline attribute everywhere. I can see why someone might expect things to work that way, but it seems surprising to me at first glance.
> >
> > Before this change I can imagine someone distributing a static archive of bitcode which could be used to produce debug binaries and release binaries. There are some issues with that (lack of lifetime markers), but it should mostly work.
>
>
> Note that LTO runs a bunch of optimizations during the compile phase that you wouldn't get with that.


There are other issues with this workflow as well. For example, Clang at -O0 will intentionally reuse local allocas to save stack space without the backend having to track lifetime. This destroys aliasing information left and right though, so you really wouldn't want this for a mixed-use bitcode archive.

I think a much more effective technique (if folks actually want to do this) would be (with whatever flag spelling you want)

  % clang -O -emit-llvm -mllvm -disable-llvm-passes

This will generate a frontend-optimized but backend pristine bitcode file that can be processed more or less depending on the desire of the user...

But I also don't think we really need to do a ton of work to support this hypothetical. If someone really wants to make this work, they should specifically request some mode and we should surface a clear flag documented to provide it. Joerg's idea about '-emit-raw-llvm' would be a nice step in this direction IMO.

> Steven had issues in the past when mixing O0 and https://reviews.llvm.org/owners/package/2/ in LTO, I don't remember the details unfortunately (and Steven is OOO).
>  Right now we don't add the `optnone` attribute when building with O0, maybe we should as well if we go this route.

Fundamentally, yes. This patch is trying to follow the path we've been going down where we try to propagate optimization restrictions to LTO. We do this for -Os and -Oz, but not -O0. This propagates one aspect (inlining), but I would be interested in using optnone to more effectively propagate O0.

That said, I agree with Reid that it is a bit surprising. After thinking about it a lot, the reason I personally find it surprising is actually that I find -O0 being the default surprising. But I'm OK with not changing that here, and maybe never changing that at the CC1 layer.

Thoughts? Generally happy with this direction?

There is another semantic change here that I intended but Mehdi highlighted I failed to fully implement: I'm also *not* using 'noinline' at -O1, and instead just building a simpler pass pipeline. I think this better fits with the intent: -O1 vs -O2 vs -O3 don't really change the *target* of optimization but instead *how much* optimization to do. As such, they feel like they don't need to be in attributes where -O0, -Os, and -Oz do.



================
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();
----------------
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.


https://reviews.llvm.org/D28053