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

[Vitaly Buka via cfe-commits]

vitalybuka marked an inline comment as done.
vitalybuka added a comment.

Please take a look. Meanwhile, I will investigate performance footprint.

In https://reviews.llvm.org/D24693#559781, @rsmith wrote:

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


I tried to do so from beginning but seems common part is only recursive traversal of AST. Sharing this will likely make both classes less readable.
In my taste it would be easier to maintain them separately.



================
Comment at: lib/CodeGen/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;
----------------
rsmith wrote:
> Comment seems to not be relevant in this copy of the code; we don't have any special case for block literals here.
I read this comment as explanation of "origParentScope : independentParentScope" and it's still needed and  relevant.


================
Comment at: lib/CodeGen/VarBypassDetector.cpp:82-86
+  case Stmt::CaseStmtClass:
+  case Stmt::DefaultStmtClass:
+  case Stmt::LabelStmtClass:
+    LabelAndGotoScopes[S] = ParentScope;
+    break;
----------------
rsmith wrote:
> 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?
Done here, but can't do for  JumpDiagnostics. there are various BuildScopeInformation which can have label as child.


https://reviews.llvm.org/D24693