This is an archive of the discontinued LLVM Phabricator instance.

[AST] Make AliasSetTracker work on BatchAA
ClosedPublic

Authored by nikic on Nov 14 2022, 7:58 AM.

Details

Summary

Switch AliasSetTracker to use BatchAAResults instead of AAResults. This means that AliasSetTracker can only be used on IR that does not change (at least without further justification). The last user of AST with changing IR was LICM and has since been removed. Everything else just needs it in a separate analysis phase.

The primary motivation for this change is to have a single BatchAA instance on which everything in AST works, which makes a followup patch simpler.

Diff Detail

Event Timeline

nikic created this revision.Nov 14 2022, 7:58 AM
nikic requested review of this revision.Nov 14 2022, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 7:58 AM
nikic added reviewers: asbirlea, fhahn, reames.

D137958 is the patch that makes use of this. Consistently using BatchAA and enabling it for the whole BatchAA instance was the nicest way I found to expose this functionality.

I have no problem with the idea of the patch, but I think this needs to be split into two patches.

The first simply removes support for ASTs over mutating IR, and makes the API changes required. (Note the suggestion about asserting value handl inline.) This should land on it's own so that if we missed a remaining mutation point, we get a chance to smoke out the failure without reasoning about potential confusing AA results at the same time.

The second actually uses batch-aa.

llvm/lib/Analysis/AliasSetTracker.cpp
626

This bit really doesn't look right.

I suspect that the ASTCallbackVH should now be a AssertingVH. At a minimum, this needs to be a well written assert or fatal error, not an unreachable.

I have no problem with the idea of the patch, but I think this needs to be split into two patches.

The first simply removes support for ASTs over mutating IR, and makes the API changes required. (Note the suggestion about asserting value handl inline.) This should land on it's own so that if we missed a remaining mutation point, we get a chance to smoke out the failure without reasoning about potential confusing AA results at the same time.

That's a good point! I've split the patch up into two parts now, and replaced the CallbackVH use with AssertVH as well.

reames accepted this revision.Nov 15 2022, 7:23 AM

LGTM

This revision is now accepted and ready to land.Nov 15 2022, 7:23 AM
asbirlea accepted this revision.Nov 15 2022, 2:24 PM

+1 on making the AST only work on immutable IR.

The only nit from me: allow the constructor to receive AAResults and build the BatchAA instance internally? It would simplify most instantiation sites modified in the patch. It still makes sense to define the BatchAA in some cases (LoopReroll), but I don't see the purpose of keeping the instance in LoopAccessAnalysis. Downside: allocation - which might make it not worth it.

Missed the dependent patch on LoopAccessAnalysis, now it makes sense :-)

This revision was landed with ongoing or failed builds.Dec 4 2022, 11:12 PM
This revision was automatically updated to reflect the committed changes.