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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:784
 
   // If the function declares it only reads memory, go with that.
+  ModRefInfo MR = ModRefInfo::ModRef;
----------------
reames wrote:
> This bit of code is repeated from the diff above.  Seems like a candidate for a helper function.  (In a follow up change please!)
These two pieces of code work on different types (Function and CallBase) -- but it's probably possible to reuse them with a templated function...


================
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());
----------------
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.


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

https://reviews.llvm.org/D130896