Page MenuHomePhabricator

[BasicAA] Don't pass through AA metadata (NFCI)
ClosedPublic

Authored by nikic on Oct 24 2020, 8:11 AM.

Details

Summary

BasicAA itself doesn't make use of AA metadata, but passes it through to recursive queries and makes it part of the cache key. Aliasing decisions that are based on AA metadata (i.e. TBAA and ScopedAA) are based *only* on AA metadata, so checking them with different pointer values or sizes is not useful, the result will always be the same.

By itself, this change is a mild compile-time improvement (https://llvm-compile-time-tracker.com/compare.php?from=93127a4d7350261ed9ee2ccaa9c369eda2b60198&to=f4d0c5f01dc446f0c40e8dd81e6b9e9b02217c4d&stat=instructions). However, the actual goal here is to remove the AA metadata from the cache key and thus reduce it's size significantly (from 96 to 32 bytes), which will be the more impactful change. I plan to do that as a followup, but could also include it here.

Diff Detail

Event Timeline

nikic created this revision.Oct 24 2020, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2020, 8:11 AM
nikic requested review of this revision.Oct 24 2020, 8:11 AM
asbirlea accepted this revision.Oct 28 2020, 1:28 PM

This is an interesting point. IIUC, your argument is that the entry point into AA is always through AAResults and can rely on the invocation result for TBAA or ScopedAA from there, while the recursive invocations into these same analyses cannot yield stricter results. I cannot think of a case when this is not true.

This revision is now accepted and ready to land.Oct 28 2020, 1:28 PM
This revision was landed with ongoing or failed builds.Apr 3 2021, 2:31 AM
This revision was automatically updated to reflect the committed changes.

I am wondering how this will interact with the ptr_provenance changes/full restrict changes. In the full restrict version, V1AAInfo and V2AAInfo are used in BasicAAResult::aliasCheck to decide if we can see trough noalias intrinsics or not.

nikic added a comment.Apr 7 2021, 7:19 AM

@jeroen.dobbelaere I'm not sure on the details, but it may be necessary to pass through just the ptr_provenance information with full restrict and include it in the cache key (while omitting all the other metadata).

@jeroen.dobbelaere I'm not sure on the details, but it may be necessary to pass through just the ptr_provenance information with full restrict and include it in the cache key (while omitting all the other metadata).

Yes, that might be a valid path.

@jeroen.dobbelaere I'm not sure on the details, but it may be necessary to pass through just the ptr_provenance information with full restrict and include it in the cache key (while omitting all the other metadata).

Yes, that might be a valid path.

On the other hand, a ptr_provenance path skips pointer computations. We might need to refactor this code to separate the 'based on' part, so we can first check the ptr_provenance path (if available), and when that is not conclusive,
we can fall back to the original pointers (that also contain GEP information etc).