[PATCH] D130896: [AA] Tracking per-location ModRef info in FunctionModRefBehavior (NFCI)

Mon Sep 12 13:56:41 PDT 2022

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM given @nikic's explanation of why my two concerns are actually addressed.

================
Comment at: llvm/lib/Analysis/GlobalsModRef.cpp:242
 FunctionModRefBehavior GlobalsAAResult::getModRefBehavior(const Function *F) {
-  FunctionModRefBehavior Min = FMRB_UnknownModRefBehavior;
+  if (FunctionInfo *FI = getFunctionInfo(F))
+    return FunctionModRefBehavior::anyLoc(FI->getModRefInfo());
----------------
nikic wrote:
> reames wrote:
> > This change doesn't look nfc.  Previously, the FunctionInfo could only improve AA results on the function.  Now, you can get less precise results if you have a FunctionInfo.
> The relevant detail here is that `AAResultBase::getModRefBehavior(F)` actually always returns `FunctionModRefBehavior::unknown()`, so there is no point in intersecting with it.
> 
> The reason why the code was written the way it was is probably a historical leftover: I believe that at some point in the past, this was how AA providers were chained. However, nowadays a different layer is responsible for performing the intersection (`AAResults`). Individual AA providers only return the best result they can produce based on their own information. It will then get intersected with the results from other AA providers by AAResults.
> 
> The `AAResultBase` methods are only default implementations returning the conservative result for the case where an AA provider does not wish to override a specific method.
> 
> I can land the removal of the intersection separately to make it more obvious that this patch doesn't change any behavior.
If you could, I'd appreciate it.

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

https://reviews.llvm.org/D130896