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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
