================
@@ -2856,8 +2873,29 @@ void ExprEngine::processBranch(
// conflicts with the widen-loop analysis option (which is off by
// default). If we intend to support and stabilize the loop widening,
// we must ensure that it 'plays nicely' with this logic.
- if (!SkipTrueBranch || AMgr.options.ShouldWidenLoops)
+ if (!SkipTrueBranch || AMgr.options.ShouldWidenLoops) {
Builder.generateNode(StTrue, true, PredN);
+ } else if (AMgr.options.LegacyInliningPrevention) {
+ // FIXME: There is an ancient and very arbitrary heuristic in
+ // `ExprEngine::processCFGBlockEntrance` which prevents all further
+ // inlining of a function if it finds an execution path within that
+ // function which reaches the `MaxBlockVisitOnPath` limit (a/k/a
+ // `analyzer-max-loop`, by default four iterations in a loop). Adding
+ // this "don't assume third iteration" logic significantly increased
+ // the analysis runtime on some inputs because less functions were
+ // arbitrarily excluded from being inlined, so more entrypoints used
+ // up their full allocated budget. As a hacky compensation for this,
+ // here we apply the "should not inline" mark in cases when the loop
+ // could potentially reach the `MaxBlockVisitOnPath` limit without the
+ // "don't assume third iteration" logic. This slightly overcompensates
+ // (activates if the third iteration can be entered, and will not
+ // recognize cases where the fourth iteration would't be completed),
but
+ // should be good enough for practical purposes.
+ if (const LocationContext *LC = getInlinedLocationContext(Pred, G)) {
+ Engine.FunctionSummaries->markShouldNotInline(
+ LC->getStackFrame()->getDecl());
+ }
+ }
----------------
NagyDonat wrote:
> My point is, if Ericsson will ship this true, and we are also going to ship
> this with true, then who would want to disable this? Do you expect users
> disabling this?
Good point.
> Would anyone complain if we would just land this fix without an option for
> opting out?
I felt that the logic introduced by this commit is so ugly that it should not
be unconditionally part of the analyzer behavior (especially if it is `git
blame`d to me...). By introducing this option I wanted to express that "this is
not a stable permanent part of the analyzer, please don't rely on its
existence" -- and I felt that yet another "FIXME" comment is not enough for
this hack.
However, I can accept removing this option (and making the hack unconditional)
if you strongly prefer that.
> Also I am not sure we should have legacy in the name of something that is
> enabled by default.
When I wrote this change, I hoped that this config option could be switched off
and deprecated in the foreseeable future (in fact it was disabled by default in
the first draft of the commit).
https://github.com/llvm/llvm-project/pull/136720
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
