[PATCH] D24693: [CodeGen] Don't emit lifetime intrinsics for some local variables

[Richard Smith via cfe-commits]

rsmith added a comment.

OK, so it seems like all the other approaches we discussed have problems too. Let's move forward with this for now.

Is there some reasonable base set of functionality between this and JumpDiagnostics that could be factored out and shared?



> VarBypassDetector.cpp:45
> +
> +/// BuildScopeInformation - Walk through the statements, adding any labels or
> +/// gotos to LabelAndGotoScopes and recursively walking the AST as needed.

Drop the `BuildScopeInformation -` here and in other documentation comments.

> VarBypassDetector.cpp:50-51
> +  // If this is a statement, rather than an expression, scopes within it don't
> +  // propagate out into the enclosing scope.  Otherwise we have to worry
> +  // about block literals, which have the lifetime of their enclosing statement.
> +  unsigned independentParentScope = origParentScope;

Comment seems to not be relevant in this copy of the code; we don't have any special case for block literals here.

> VarBypassDetector.cpp:82-86
> +  case Stmt::CaseStmtClass:
> +  case Stmt::DefaultStmtClass:
> +  case Stmt::LabelStmtClass:
> +    LabelAndGotoScopes[S] = ParentScope;
> +    break;

This looks unreachable (our only callers are the recursive call below -- which already checked for these -- and cases that cannot have a labelled statement). If we did reach it, we'd do the wrong thing, because we'd have created an independent scope rather than reusing the parent's scope (`x: int n;` would not introduce a new scope into the parent for `n`). Maybe replace this case with `llvm_unreachable`, or move the `while (true)` loop below up to the top of this function and delete these cases.

Do we have the same issue in JumpDiagnostics?

> VarBypassDetector.cpp:105-108
> +      if (const CaseStmt *CS = dyn_cast<CaseStmt>(SubStmt))
> +        Next = CS->getSubStmt();
> +      else if (const DefaultStmt *DS = dyn_cast<DefaultStmt>(SubStmt))
> +        Next = DS->getSubStmt();

Combine these two into a single `if (auto *SC = dyn_cast<SwitchCase>(SubStmt))` case.

> VarBypassDetector.cpp:129
> +      if (const LabelStmt *LS = GS->getLabel()->getStmt())
> +        Detect(GS, LS);
> +    } else if (const SwitchStmt *SS = dyn_cast<SwitchStmt>(St)) {

Maybe pass in `S.second` instead of `GS` here so that the callee doesn't need to look it up again.

> VarBypassDetector.h:39
> +///  * Not limited to variables with initializers, JumpScopeChecker is limited.
> +///  * FIXME: Does not support indirect jumps.
> +class VarBypassDetector {

We should do /something/ about these. We could track the set of address-taken labels when we walk the function, and assume that each indirect goto can jump to each address-taken label, or more simply just conservatively say that all variables are bypassed in a function that contains an indirect goto.

https://reviews.llvm.org/D24693