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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Generally, this makes sense to me.  However, reading through, I spot a couple things which don't look NFC.

Can we find a way to stage this change?  Here's one idea, but I'm open to others as well.

1. Leave the AliasAnalysis accessors for the moment, canonicalize code to use them rather than direct comparison of FRM.  (This just shrinks the patch.)
2. Wrap the existing enum in a class of the same name.  Add the accessor functions, and use them where obvious.
3. Move the non-obvious ones into their own reviews.
4. Change the representation of the class.

The things I spotted are minor, so I can be convinced to approve without splitting, but I would split if it were my patch, if only to convince myself I'd gotten it right.



================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:784
 
   // If the function declares it only reads memory, go with that.
+  ModRefInfo MR = ModRefInfo::ModRef;
----------------
This bit of code is repeated from the diff above.  Seems like a candidate for a helper function.  (In a follow up change please!)


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


================
Comment at: llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp:419
 
-  return FunctionModRefBehavior(AAResultBase::getModRefBehavior(Call) & Min);
+  return AAResultBase::getModRefBehavior(Call);
 }
----------------
Same problem as above.  Imagine the call was readnone.  


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

https://reviews.llvm.org/D130896