[FunctionAttrs] Replace MemoryAccessKind with FMRB.

[fhahn]

Summary

Update FunctionAttrs to use FunctionModRefBehavior instead
MemoryAccessKind.

This allows for adding support for inferring argmemonly and others,
see D121415.

[nikic]

Hm, could we maybe replace MemoryAccessKind with FunctionModRefBehavior? That's basically ArgMemOnly + InaccessibleMemOnly + ModRef, which seems like where we'll want to end up here as well?

[fhahn]

In D121460#3375133, @nikic wrote:

Hm, could we maybe replace MemoryAccessKind with FunctionModRefBehavior? That's basically ArgMemOnly + InaccessibleMemOnly + ModRef, which seems like where we'll want to end up here as well?

Yeah that seems to work and overall less work. I updated this patch to replace MemoryAccessKind with FunctionModRefBehavior.

[reames]

It looks like some changes to support argmemonly made into this patch. If you want to remove those, and land the refactoring, LGTM to that.

[nikic]

llvm/include/llvm/Transforms/IPO/FunctionAttrs.h

23

Why?

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

131–132

I think you can just return MRB in this whole branch.

183

This is presumably not supposed to be part of this patch, as it's not used?

[fhahn]

In D121460#3375447, @reames wrote:

It looks like some changes to support argmemonly made into this patch. If you want to remove those, and land the refactoring, LGTM to that.

Oh yes, I removed those and the stale cstdint include.

[fhahn]

llvm/include/llvm/Transforms/IPO/FunctionAttrs.h

23

leftover, removed

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

131–132

updated

183

Yes, that made it in by accident. Removed

[nikic]

LGTM

llvm/include/llvm/Transforms/IPO/FunctionAttrs.h

 /// Provides passes for computing function attributes based on interprocedural
 /// analyses.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_IPO_FUNCTIONATTRS_H
 #define LLVM_TRANSFORMS_IPO_FUNCTIONATTRS_H
 
+#include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/CGSCCPassManager.h"
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 
 class AAResults;
 class Function;
 class Module;
 class Pass;
 
-/// The three kinds of memory access relevant to 'readonly' and
-/// 'readnone' attributes.
-enum MemoryAccessKind {
-  MAK_ReadNone = 0,
-  MAK_ReadOnly = 1,
-  MAK_MayWrite = 2,
-  MAK_WriteOnly = 3
-};
-
 /// Returns the memory access properties of this copy of the function.
-MemoryAccessKind computeFunctionBodyMemoryAccess(Function &F, AAResults &AAR);
+FunctionModRefBehavior computeFunctionBodyMemoryAccess(Function &F,
+                                                       AAResults &AAR);
 
 /// Propagate function attributes for function summaries along the index's
 /// callgraph during thinlink
 bool thinLTOPropagateFunctionAttrs(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing);
 

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

 /// Returns the memory access attribute for function F using AAR for AA results,
 /// where SCCNodes is the current SCC.
 ///
 /// If ThisBody is true, this function may examine the function body and will
 /// return a result pertaining to this copy of the function. If it is false, the
 /// result will be based only on AA results for the function declaration; it
 /// will be assumed that some other (perhaps less optimized) version of the
 /// function may be selected at link time.
-static MemoryAccessKind checkFunctionMemoryAccess(Function &F, bool ThisBody,
-                                                  AAResults &AAR,
-                                                  const SCCNodeSet &SCCNodes) {
+static FunctionModRefBehavior
+checkFunctionMemoryAccess(Function &F, bool ThisBody, AAResults &AAR,
   FunctionModRefBehavior MRB = AAR.getModRefBehavior(&F);
   if (MRB == FMRB_DoesNotAccessMemory)
     // Already perfect!
-    return MAK_ReadNone;
+    return MRB;
 
-  if (!ThisBody) {
-    if (AliasAnalysis::onlyReadsMemory(MRB))
-      return MAK_ReadOnly;
-
-    if (AliasAnalysis::onlyWritesMemory(MRB))
-      return MAK_WriteOnly;
-
-    // Conservatively assume it reads and writes to memory.
-    return MAK_MayWrite;
-  }
+  if (!ThisBody)
+    return MRB;
 
   // Scan the function body for instructions that may read or write memory.
   bool ReadsMemory = false;
   bool WritesMemory = false;
   for (Instruction &I : instructions(F)) {
     // Some instructions can be ignored even if they read or write memory.
     // Detect these now, skipping to the next instruction if one is found.
     if (auto *Call = dyn_cast<CallBase>(&I)) {
if (auto *Call = dyn_cast<CallBase>(&I)) {
       for (const Use &U : Call->args()) {
         const Value *Arg = U;
         if (!Arg->getType()->isPtrOrPtrVectorTy())
           continue;
 
         MemoryLocation Loc =
             MemoryLocation::getBeforeOrAfter(Arg, I.getAAMetadata());
 
         // Skip accesses to local or constant memory as they don't impact the
         // externally visible mod/ref behavior.
         if (AAR.pointsToConstantMemory(Loc, /*OrLocal=*/true))
           continue;
 
         if (isModSet(MRI))
           // Writes non-local memory.
           WritesMemory = true;
         if (isRefSet(MRI))
for (Instruction &I : instructions(F)) {
     //
     // Writes memory, remember that.
     WritesMemory |= I.mayWriteToMemory();
 
     // If this instruction may read memory, remember that.
     ReadsMemory |= I.mayReadFromMemory();
   }
 
-  if (WritesMemory) { 
+  if (WritesMemory) {
     if (!ReadsMemory)
-      return MAK_WriteOnly;
+      return FMRB_OnlyWritesMemory;
     else
-      return MAK_MayWrite;
+      return FMRB_UnknownModRefBehavior;
   }
 
-  return ReadsMemory ? MAK_ReadOnly : MAK_ReadNone;
+  return ReadsMemory ? FMRB_OnlyReadsMemory : FMRB_DoesNotAccessMemory;
 }
 
-MemoryAccessKind llvm::computeFunctionBodyMemoryAccess(Function &F,
+FunctionModRefBehavior llvm::computeFunctionBodyMemoryAccess(Function &F,
                                                              AAResults &AAR) {
   return checkFunctionMemoryAccess(F, /*ThisBody=*/true, AAR, {});
 }
 
 /// Deduce readonly/readnone/writeonly attributes for the SCC.
 template <typename AARGetterT>
 static void addMemoryAttrs(const SCCNodeSet &SCCNodes, AARGetterT &&AARGetter,
                            SmallSet<Function *, 8> &Changed) {
   // Check if any of the functions in the SCC read or write memory.  If they
   // write memory then they can't be marked readnone or readonly.
   bool ReadsMemory = false;
   bool WritesMemory = false;
   for (Function *F : SCCNodes) {
     // Call the callable parameter to look up AA results for this function.
     AAResults &AAR = AARGetter(*F);
 
     // Non-exact function definitions may not be selected at link time, and an
     // alternative version that writes to memory may be selected.  See the
     // comment on GlobalValue::isDefinitionExact for more details.
-    switch (checkFunctionMemoryAccess(*F, F->hasExactDefinition(),
-                                      AAR, SCCNodes)) {
-    case MAK_MayWrite:
+    FunctionModRefBehavior FMRB =
+        checkFunctionMemoryAccess(*F, F->hasExactDefinition(), AAR, SCCNodes);
+    if (isModAndRefSet(createModRefInfo(FMRB)))
       return;
-    case MAK_ReadOnly:
-      ReadsMemory = true;
-      break;
-    case MAK_WriteOnly:
-      WritesMemory = true;
-      break;
-    case MAK_ReadNone:
-      // Nothing to do!
-      break;
-    }
+    if (FMRB == FMRB_DoesNotAccessMemory)
+      continue;
+    ReadsMemory |= AliasAnalysis::onlyReadsMemory(FMRB);
+    WritesMemory |= AliasAnalysis::onlyWritesMemory(FMRB);
   }
 
   // If the SCC contains both functions that read and functions that write, then
   // we cannot add readonly attributes.
   if (ReadsMemory && WritesMemory)
     return;
 
   // Success!  Functions in this SCC do not access memory, or only read memory.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

if (HasTypeMetadata(&GV)) {
             !F->arg_begin()->use_empty())
           return;
         for (auto &Arg : drop_begin(F->args())) {
           auto *ArgT = dyn_cast<IntegerType>(Arg.getType());
           if (!ArgT || ArgT->getBitWidth() > 64)
             return;
         }
         if (!F->isDeclaration() &&
-            computeFunctionBodyMemoryAccess(*F, AARGetter(*F)) == MAK_ReadNone)
+            computeFunctionBodyMemoryAccess(*F, AARGetter(*F)) ==
+                FMRB_DoesNotAccessMemory)
           EligibleVirtualFns.insert(F);
       });
     }
 
   ValueToValueMapTy VMap;
   std::unique_ptr<Module> MergedM(
       CloneModule(M, VMap, [&](const GlobalValue *GV) -> bool {
         if (const auto *C = GV->getComdat())

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

bool DevirtModule::tryVirtualConstProp(
   // than testing function attributes, which must hold for any copy of the
   // function, even a less optimized version substituted at link time. This is
   // sound because the virtual constant propagation optimizations effectively
   // inline all implementations of the virtual function into each call site,
   // rather than using function attributes to perform local optimization.
   for (VirtualCallTarget &Target : TargetsForSlot) {
     if (Target.Fn->isDeclaration() ||
         computeFunctionBodyMemoryAccess(*Target.Fn, AARGetter(*Target.Fn)) !=
-            MAK_ReadNone ||
+            FMRB_DoesNotAccessMemory ||
         Target.Fn->arg_empty() || !Target.Fn->arg_begin()->use_empty() ||
         Target.Fn->getReturnType() != RetType)
       return false;
   }
 
   for (auto &&CSByConstantArg : SlotInfo.ConstCSInfo) {
     if (!tryEvaluateFunctionsWithArgs(TargetsForSlot, CSByConstantArg.first))
       continue;