This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Add Basic Escape Analysis
AbandonedPublic

Authored by JDevlieghere on Nov 15 2016, 10:55 AM.

Details

Reviewers
chandlerc
hfinkel
Summary

At GuardSquare we make extensive use of LLVM. Recently we were in need for Escape Analysis and to our surprise it was not part of LLVM. A quick search revealed that it was removed due to a bug (infinite loop) back in 2008. We came up with our own implementation which we would gladly make available to the community and upstream. The implementation has worked well so far for our need, but it is by no means complete, as it does not currently account for thread-related escapes to name an example. However, our hope is that, if there's interest from the community, it can serve as a starting point for further development as part of LLVM.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere retitled this revision from to [analyzer] Add Basic Escape Analysis.
JDevlieghere updated this object.
JDevlieghere added reviewers: dcoughlin, NoQ, zaks.anna.
JDevlieghere set the repository for this revision to rL LLVM.
JDevlieghere added a subscriber: dzn.
zaks.anna edited edge metadata.Nov 15 2016, 2:54 PM

This is LLVM patch, while the clang static analyzer works on clang AST level.

zaks.anna edited reviewers, added: chandlerc; removed: zaks.anna, NoQ, dcoughlin.Nov 15 2016, 2:55 PM
zaks.anna removed rL LLVM as the repository for this revision.
zaks.anna added a subscriber: llvm-commits.
zaks.anna added a subscriber: zaks.anna.

Added llvm-commits list as per http://llvm.org/docs/Phabricator.html.

Chandler, do you know who could review this patch?

JDevlieghere retitled this revision from [analyzer] Add Basic Escape Analysis to [Analysis] Add Basic Escape Analysis.Nov 15 2016, 10:32 PM
JDevlieghere set the repository for this revision to rL LLVM.

Ping. If anyone knows the right people to review this patch, please let me know!

hfinkel edited edge metadata.Dec 1 2016, 4:58 PM

How does this differ from our capture tracking (include/llvm/Analysis/CaptureTracking.h and lib/Analysis/CaptureTracking.cpp)?

reames edited edge metadata.Dec 1 2016, 5:58 PM
reames added subscribers: apilipenko, anna.
reames added a subscriber: reames.
reames added a comment.Dec 1 2016, 6:00 PM

Can you give a bit of context on the expected use of this code? In particular, how it differs from the existing capture tracking analysis which is used by various parts of the optimizer? I'm generally in support of improving LLVM's escape analysis capabilities, I'm just looking for a bit more information.

p.s. There are a bunch of stylistic issues with the code which will need corrected. I am not going to review carefully until the meta issue is settled.

Hal, Philip, thanks for having a look. Because you both mentioned CaptureTracking.cpp, I took another look at the source code and I'm starting to doubt my original premise that there are cases where we have something that escapes but isn't captured. I'll do some testing over the weekend to make sure.

Hal, Philip, thanks for having a look. Because you both mentioned CaptureTracking.cpp, I took another look at the source code and I'm starting to doubt my original premise that there are cases where we have something that escapes but isn't captured. I'll do some testing over the weekend to make sure.

I'd also be interested in the opposite situations: Are there cases where things are captured but don't escape and that's uncovered by your analysis. I'm under the impressions that some of our capturing checks are really escape checks and more precision could be useful.

I ran our local tests against the CaptureTracker and everything is properly detected. Most of our cases involve stores and the current implementation is very conservative in that it always assumes a store captures.

The code also contains the following TODO, which I think I might be able to address with my implementation.

// TODO: If StoreCaptures is not true, we could do Fancy analysis
// to determine whether this store is not actually an escape point.
// In that case, BasicAliasAnalysis should be updated as well to
// take advantage of this.

I only considers a values to be escaped if the it is stored to the enclosing function's arguments or to a global. I rely on AliasAnalysis to check that the pointer might be an alias to these values. Because AliasAnalysis depends on CaptureTracking, I see two possibilities:

  1. Keeping the current interface and not using AliasAnalysis, but comparing for equality rather than checking for "might alias". This is not really conservative, though arguably what you might expect from setting StoreCapture to false?
  2. Adding a new interface where the caller provides alias analysis info, like what's currently done for the dominator tree. The AliasAnalysis implementation could continue using the current implementation where a store is assumed to capture.

What do you guys think?

PS: I'm not entirely sure what is meant by the last sentence in the TODO though, so I might be missing something. I'm doing my best to get a better understanding of how all of this works in LLVM, so please don't hesitate to point out if anything I mentioned doesn't make sense.

I ran our local tests against the CaptureTracker and everything is properly detected. Most of our cases involve stores and the current implementation is very conservative in that it always assumes a store captures.

The code also contains the following TODO, which I think I might be able to address with my implementation.

// TODO: If StoreCaptures is not true, we could do Fancy analysis
// to determine whether this store is not actually an escape point.
// In that case, BasicAliasAnalysis should be updated as well to
// take advantage of this.

I only considers a values to be escaped if the it is stored to the enclosing function's arguments or to a global. I rely on AliasAnalysis to check that the pointer might be an alias to these values. Because AliasAnalysis depends on CaptureTracking, I see two possibilities:

  1. Keeping the current interface and not using AliasAnalysis, but comparing for equality rather than checking for "might alias". This is not really conservative, though arguably what you might expect from setting StoreCapture to false?
  2. Adding a new interface where the caller provides alias analysis info, like what's currently done for the dominator tree. The AliasAnalysis implementation could continue using the current implementation where a store is assumed to capture.

What do you guys think?

Our current use of capture tracking in AA is not very efficient (especially on large basic blocks, although the OrderedBasicBlock cache has made this a bit better). Using a pre-computed analysis could be significantly better.

However, I'm not sure that I understand how this would work. Generally, the uses in AA care about whether or not a pointer has been captured before some point:

ptr2 = ...;
...
ptr1 = foo();
x = *ptr1;
y = *ptr2; // might *ptr1 and *ptr2 alias here? It might depend on whether ptr2 is captured before the call to foo(). If it was, then foo() might return the pointer somehow. If not, then we know that ptr2 does not alias with ptr1.

Would your escape analysis tell us this?

PS: I'm not entirely sure what is meant by the last sentence in the TODO though, so I might be missing something. I'm doing my best to get a better understanding of how all of this works in LLVM, so please don't hesitate to point out if anything I mentioned doesn't make sense.

In reply to @reames e-mail: I don't know what particular transformation would benefit from this change, but I can hardly imagine that making it more accurate is a bad thing. Consider the following example:

define void @sample() {
entry:
  %ptr = alloca i32
  store i32 1, i32* %ptr
  %ptrtoptr = alloca i32*
  store i32* %ptr, i32** %ptrtoptr
  ret void
}

The current implementation of CaptureTracking says that %ptr escapes because it's the operand of the second store. What I propose is to only consider %ptr to escape if it's stored to either a global or to one of the arguments of the function. Neither is the case in the example, so it would not be considered escaped. So why do we need AliasAnalysis? Because %ptr might be stored to another pointer that's aliasing a function argument or global. With option (2), AA would continue working as it does today (considering any store to capture), but other passes that rely on AA anyway, could choose to use the more optimistic implementation.

Since I don't think that option (2) adds much code, I'll create a new differential to show what I mean exactly. Once we agree on whether this is a good idea or not we can look into the other improvements you guys mentioned such as the lazy caching and making the optimistic algorithm context-sensitive. I think it would be good to split those in separate changes anyway.

JDevlieghere abandoned this revision.Dec 8 2016, 11:45 AM

I'm abandoning this differential in favor of https://reviews.llvm.org/D27585.

yaozhongxiao removed a subscriber: yaozhongxiao.