[PATCH] D16403: Add scope information to CFG

[Peter Szecsi via Phabricator via llvm-commits]

szepet added a comment.

In https://reviews.llvm.org/D16403#992452, @NoQ wrote:

> Thank you, this explanation looks very reasonable.
>
> All right, so right after the termination of the loop we have
>
>   [B1]
>   1: ForStmt (LoopExit)
>   2: [B4.5].~A() (Implicit destructor)
>   3: [B5.3].~A() (Implicit destructor)
>   4: CFGScopeEnd(a)
>   5: CFGScopeEnd(b)
>
>
> ... where `[B4.5]` is `A b = a;` and `[B5.3]` is `A a;`. Am i understanding correctly that while destroying `a` you can still use the storage of `b` safely? Or should `a` go out of scope before `b` gets destroyed? Also, is the order of scope ends actually correct here - shouldn't `b` go out of scope earlier? Given that they are in very different lifetime scopes (`a` is one for the whole loop, `b` is per-loop-iteration). I guess the order would matter for the analyzer.


I guess CFGScopeEnd should happen (should be encountered) before the LoopExit and CFGScopeBegin should be after LoopEntrance (still not sure if it is going to be part of the CFG ever.) Just like parenthesis {(())}  that is what would make the most sense for me. However, I do not want to block this patch or something so I can do these changes as well by modifying LoopExit (and probably should since ImplicitDestructor calls should precede the LoopExit as well).

In https://reviews.llvm.org/D16403#992454, @NoQ wrote:

> @szepet: so i see that `LoopExit` goes in the beginning of the cleanup block after the loop, while various `ScopeEnd`s go after the `LoopExit`. Would loop unrolling be significantly broken if you simply subscribe to `ScopeEnd` instead of `LoopExit` and avoid cleaning up the loop state until destructors are processed? I might not be remembering correctly - is `LoopExit` only used for cleanup, or do we have more work to be done here?


I guess your following comment just answers this:

In https://reviews.llvm.org/D16403#1001466, @NoQ wrote:

> We still don't have scopes for segments of code that don't have any variables in them, so i guess it's not yet in the shape where it is super useful for loops, but it's already useful for finding use of stale stack variables, which was the whole point originally, so i think this should definitely land soon.


It could be, however, we would lose cases like:

  int i = 0;
  int a[32];
  for(i = 0;i<32;++i) {a[i] = i;}

Since there is no variable which has the scope of the loop, ScopeEnd would be not enough. Sure, we could remove this case, however, the aim is to extend the loop-patterns for completely unrolling. Another thing that there are the patches which would enhance the covered cases by LoopExit (https://reviews.llvm.org/D39398) and add the LoopEntrance to the CFG(https://reviews.llvm.org/D41150) as well. So, at this point, I feel like it would be a huge step back not to use these elements. (Sorry Maxim that we are discussing this here^^)


Repository:
  rL LLVM

https://reviews.llvm.org/D16403