[PATCH] D27585: [CaptureTracking] Add optimistic capture tracker for stores

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 27 13:19:03 PST 2016


JDevlieghere added a comment.

In https://reviews.llvm.org/D27585#630030, @sanjoy wrote:

> In https://reviews.llvm.org/D27585#623685, @JDevlieghere wrote:
>
> > Let's consider the following table:
> >
> > |                             | No AA               | Basic AA            | Advanced AA              |
> > | --------------------------- | ------------------- | ------------------- | ------------------------ |
> > | Current Capture Tracking    | false positives (a) | n/a                 | n/a                      |
> > | Optimistic Capture Tracking | false positives (b) | false negatives (c) | less false positives (d) |
> >
> > Right now, only situation **(a)** exists, where we get false positives for stores to non-aliasing function arguments and globals, as illustrated by the example in the original description of this patch. Optimistic capture tracking behaves exactly the same if no AA is provided, so situation **(a)** and **(b)** are identical.
> >
> > The interesting cases are **(c)** and **(d)**, especially in combination with the situation you described, where a "value is stored into some alloca then read and the read value stored into some global".
> >
> > - For **(c)**,  basic AA doesn't perform memory-based data-flow tracking and it returns a false negative, which in turn causes the optimistic capture tracker to return a false negative as well. Returning a false negative is less desirable than returning a false positive, i.e. situation **(a)**. So I suggest we enforce that this scenario can not occur, e.g. with an assert.
>
>
> What would you assert?  That is, what would the assert look like?


Originally I wanted to assert the algorithm used for AA. However, now I understand that unless the AA algorithm is perfect, there will always be a case where this generates a false negative. What I'd need for this to work properly is for AA to provide a guaranteed **must not alias** result.  Unfortunately for me, this is inherently incompatible with what we have now. LLVM is (correctly) conservative in saying that something aliases, while I would need an implementation that is conservative in saying that something does not alias.

>> - For **(d)**, it properly detects that the two variables might alias, and we reduce the set of false positives generated by stores to globals and function argument. The lit tests verify this.
> 
> Which lit test?

`captureindirectstore.ll`

> In any case we can't rely on the AA being smart or advanced for correctness.  That is, a less aggressive / more conservative AA should not cause us to miscompile code.

Of course, this goes without saying!

> 
> 
>> The net result is that we ether end up in **(a)** or **(c)** which is in my opinion more desirable than what we currently have.
> 
> Optimizations based on (c) will generally be incorrect and so (a) or (c) is less desirable than (a).  That is, (a) == "always correct", (a) or (c) == "sometimes wrong".

This was a typo on my part, it should've said **(a)** or **(d)**. However, it doesn't really matter anymore, because of the reason I mentioned above.

> I think Hal's question was more of the tune of: do you have optimizations that will be correct even with false negatives?  That is, even if the optimization thinks a value did not escape when it actually did, the transform it does will be be correct?  If so, that is interestingly different from what LLVM does today, and worth discussion _before_ we add this new capture tracker to LLVM.

No, I don't have an optimization that can deal with a false negative. I originally extended the capture tracker because I ran into a lot of false negatives for my use cases. This extension made sense from a theoretical point of view, and after testing it, I was able to correctly classify every case I could come up with. I think I confused both you and myself by calling it optimistic. I didn't mean for it to be optimistic in the sense that it would misclassify captures, rather that it could do better than what we currently have with the help of AA. Anyway, now I understand that I can't guarantee my desired scenario **(d)**.  I don't really see a way around this limitation, without implementing a inverse-conservative variant of AA.

Do you guys have an alternative idea for reducing the amount of false positives? If not I guess I'll have to abandon this change. :-(

In https://reviews.llvm.org/D27585#630064, @hfinkel wrote:

> Yes, this is exactly what I'd like to know.


Thank you both for the clarification. I guess I must have misunderstood the question, sorry!


Repository:
  rL LLVM

https://reviews.llvm.org/D27585





More information about the llvm-commits mailing list