This is an archive of the discontinued LLVM Phabricator instance.

[AA] Move earliest escape tracking from DSE to AA
ClosedPublic

Authored by nikic on Sep 23 2021, 2:28 PM.

Details

Summary

This is a followup to D109844 (and alternative to D109907), which integrates the new "earliest escape" tracking into AliasAnalysis. This is done by replacing the pre-existing context-free capture cache in AAQueryInfo with a replaceable (virtual) object with two implementations: The SimpleCaptureInfo implements the previous behavior (check whether object is captured at all), while EarliestEscapeInfo implements the new behavior from DSE.

This combines the "earliest escape" analysis with the full power of BasicAA: It subsumes the call handling from D109907, considers a wider range of escape sources, a wider context, and works with AA recursion.

It does come at a slightly higher compile-time cost: http://llvm-compile-time-tracker.com/compare.php?from=1e3c6fc7cb9d2ee6a5328881f95d6643afeadbff&to=23296d5394689e40a0b4376f080cfab18c43442c&stat=instructions (The comparison point would be https://llvm-compile-time-tracker.com/compare.php?from=7faf1285f2c416494aacf7ee13e70e330b2f0540&to=a4964cf36bb0f777d82d6a1025435bbd9f67bc4f&stat=instructions for D109907).

Diff Detail

Event Timeline

nikic created this revision.Sep 23 2021, 2:28 PM
nikic requested review of this revision.Sep 23 2021, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 2:28 PM
fhahn added a comment.Sep 24 2021, 1:58 AM

This combines the "earliest escape" analysis with the full power of BasicAA:

Are there any test cases that show the benefit of that, other than the improvements for the load-load case discussed in D109844 ?

nikic updated this revision to Diff 374927.Sep 24 2021, 12:27 PM

Rebase over additional tests.

nikic updated this revision to Diff 374933.Sep 24 2021, 12:47 PM

Fix unit test build.

This combines the "earliest escape" analysis with the full power of BasicAA:

Are there any test cases that show the benefit of that, other than the improvements for the load-load case discussed in D109844 ?

See the last three tests in captures-before-load.ll. Only the last one truly requires BasicAA integration though.

Ultimately my main interest here is to integrate this functionality in a way that does not require reimplementing parts of AA in DSE. Better analysis quality is just a nice side effect.

fhahn accepted this revision.Sep 25 2021, 12:18 PM

This combines the "earliest escape" analysis with the full power of BasicAA:

Are there any test cases that show the benefit of that, other than the improvements for the load-load case discussed in D109844 ?

See the last three tests in captures-before-load.ll. Only the last one truly requires BasicAA integration though.

Great thanks!

Ultimately my main interest here is to integrate this functionality in a way that does not require reimplementing parts of AA in DSE. Better analysis quality is just a nice side effect.

Agreed, just wanted to double check w.r.t. the description.

LGTM, I am just wondering if the change in isEscapeSource is required or can be split off?

llvm/include/llvm/Analysis/AliasAnalysis.h
383

nit: could just be a struct?

407

I should have added doc-comments in the original DSE version but unfortunately did not. Might be good to add some here.

llvm/lib/Analysis/BasicAliasAnalysis.cpp
122–123

this seems unrelated or could at least be split off?

This revision is now accepted and ready to land.Sep 25 2021, 12:18 PM
This revision was landed with ongoing or failed builds.Sep 25 2021, 1:47 PM
This revision was automatically updated to reflect the committed changes.
nikic marked 3 inline comments as done.
hans added a subscriber: hans.Nov 15 2021, 5:23 AM

A Chromium developer reported compiler non-determinism that we bisected to this change. see https://bugs.chromium.org/p/chromium/issues/detail?id=1268247#c12 for a reproducer with preprocessed source.

hans added a comment.Nov 18 2021, 11:51 AM

A Chromium developer reported compiler non-determinism that we bisected to this change. see https://bugs.chromium.org/p/chromium/issues/detail?id=1268247#c12 for a reproducer with preprocessed source.

Using that reproducer, it seems the non-deterministic output is coming from DSE: sometimes a store before a call is removed, and sometimes not (see comment at https://bugs.chromium.org/p/chromium/issues/detail?id=1268247#c16)

ychen added a subscriber: ychen.Nov 18 2021, 4:19 PM

I believe this is caused by non-determinism in DT child order, which is fixed by D110292.

nikic added a comment.Dec 1 2021, 12:45 AM

@hans As the DT patch landed, could you please confirm whether the non-determinism is fixed on your side? I'm no longer able to reproduce it with the test case at least.

hans added a comment.Dec 1 2021, 6:02 AM

@hans As the DT patch landed, could you please confirm whether the non-determinism is fixed on your side? I'm no longer able to reproduce it with the test case at least.

Thanks! Confirmed that it looks fixed on our end.