[PATCH] D153511: [BasicAA] Don't short-circuit non-capturing arguments

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D153511#4442640 <https://reviews.llvm.org/D153511#4442640>, @fhahn wrote:

> In D153511#4442568 <https://reviews.llvm.org/D153511#4442568>, @nikic wrote:
>
>> CaptureTracking handles both captures and escapes. There is no escape in this case. No escape does //not// imply that memory is not accessed, just that provenance is not //retained// after the call.
>>
>> BasicAA is making assumptions about how the CaptureTracking implementation works here, which goes beyond the actual capture/escape properties. Those assumptions are close, but not quite correct.
>
> Right, IIUC the code in BasicAA assumes escaping pointers to not be visible outside the current function, which includes not being passed to readonly functions.

Not really. A call can access an alloca in one of two ways: Either by the pointer first escaping and the call accessing the escaped pointer, or by the pointer being passed as an argument to the call. BasicAA performs checks for both of these. The first is based on CaptureTracking, and the second based on alias checks on the call arguments. This is the correct way to use the CaptureTracking API.

The only problem is that BasicAA, as an optimization, does not perform the argument check for non-nocapture arguments, on the premise that CaptureTracking will have treated these as an escape and thus failed the first check already.

> I was initially (incorrectly) assuming this is what our APIs would consider an escape. It might be worth to have an option to track this property for AA.

I don't think this is really possible. For the "escaped before call" check, we are only interested in escapes, not in accesses by calls. It's okay if the pointer gets accessed by previous calls, as long as it does not escape. For accessed, we only care about the current call. So this would require a call-specific result which is not really compatible with EarliestEscapeInfo.

(As a side note, with the change I'm proposing here, we could replace isNotCapturedBeforeOrAt with isNotCaptureBefore, dropping the "OrAt" part. Not sure whether this would make any practical difference.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153511/new/

https://reviews.llvm.org/D153511