Address review comments

- Bail on unexpected (non CallInst) use of function
- Move checks into findArgParts to improve clarity and only run when
  necessary
- Remove FIXMEs from test

Author: hazzlim

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

@@ -485,12 +485,39 @@ static bool allCallersPassValidPointerForArgument(
   });
 }
 
+// Try to prove that all Calls to F do not modify the memory pointed to by Arg,
+// using alias analysis local to each caller of F.
+static bool isArgUnmodifiedByAllCalls(Function *F, unsigned ArgNo,
+                                      FunctionAnalysisManager &FAM) {
+  // Check if all Users of F are Calls which do not modify Arg.
+  for (User *U : F->users()) {
+
+    // Bail if we find an unexpected (non CallInst) use of the function.
+    auto *Call = dyn_cast<CallInst>(U);
+    if (!Call)
+      return false;
+
+    Value *ArgOp = Call->getArgOperand(ArgNo);
+    assert(ArgOp->getType()->isPointerTy() && "Argument must be Pointer Type!");
+
+    MemoryLocation Loc = MemoryLocation::getForArgument(Call, ArgNo, nullptr);
+
+    AAResults &AAR = FAM.getResult<AAManager>(*Call->getFunction());
+    // Bail as soon as we find a Call where Arg may be modified.
+    if (isModSet(AAR.getModRefInfo(Call, Loc)))
+      return false;
+  }
+
+  // All Users are Calls which do not modify the Arg.
+  return true;
+}
+
 /// Determine that this argument is safe to promote, and find the argument
 /// parts it can be promoted into.
 static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
                          unsigned MaxElements, bool IsRecursive,
                          SmallVectorImpl<OffsetAndArgPart> &ArgPartsVec,
-                         bool ArgNotModified) {
+                         FunctionAnalysisManager &FAM) {
   // Quick exit for unused arguments
   if (Arg->use_empty())
     return true;
@@ -712,17 +739,21 @@ static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
 
   // If store instructions are allowed, the path from the entry of the function
   // to each load may be not free of instructions that potentially invalidate
-  // the load, and this is an admissible situation. If we have already
-  // determined that the pointer Arg is not modified in the function (for all
-  // Calls) then we can similarly conclude analysis here.
-  if (AreStoresAllowed || ArgNotModified)
+  // the load, and this is an admissible situation.
+  if (AreStoresAllowed)
     return true;
 
   // Okay, now we know that the argument is only used by load instructions, and
-  // it is safe to unconditionally perform all of them. Use alias analysis to
-  // check to see if the pointer is guaranteed to not be modified from entry of
-  // the function to each of the load instructions.
+  // it is safe to unconditionally perform all of them.
+
+  // If we can determine that no call to the Function modifies the memory
+  // pointed to by Arg, through alias analysis using actual arguments in the
+  // callers, we know that it is guaranteed to be safe to promote the argument.
+  if (isArgUnmodifiedByAllCalls(Arg->getParent(), Arg->getArgNo(), FAM))
+    return true;
 
+  // Otherwise, use alias analysis to check if the pointer is guaranteed to not
+  // be modified from entry of the function to each of the load instructions.
   for (LoadInst *Load : Loads) {
     // Check to see if the load is invalidated from the start of the block to
     // the load itself.
@@ -763,33 +794,6 @@ static bool areTypesABICompatible(ArrayRef<Type *> Types, const Function &F,
   });
 }
 
-// Try to prove that all Calls to F do not modify the memory pointed to by Arg.
-// This can provide us with more opportunities to perform Argument Promotion in
-// cases where simply looking at a Function's instructions is insufficient to
-// prove that the pointer argument is not invalidated before all loads from it.
-static bool callDoesNotModifyArg(Function *F, unsigned ArgNo,
-                                 FunctionAnalysisManager &FAM) {
-  // Find all Users of F that are Calls, and see if they may modify Arg.
-  for (User *U : F->users()) {
-    auto *Call = dyn_cast<CallInst>(U);
-    if (!Call)
-      continue;
-
-    Value *ArgOp = Call->getArgOperand(ArgNo);
-    assert(ArgOp->getType()->isPointerTy() && "Argument must be Pointer Type!");
-
-    MemoryLocation Loc = MemoryLocation::getForArgument(Call, ArgNo, nullptr);
-
-    AAResults &AAR = FAM.getResult<AAManager>(*Call->getFunction());
-    // Bail out as soon as we find a Call where Arg may be modified.
-    if (isModSet(AAR.getModRefInfo(Call, Loc)))
-      return false;
-  }
-
-  // All Calls do not modify the Arg.
-  return true;
-}
-
 /// PromoteArguments - This method checks the specified function to see if there
 /// are any promotable arguments and if it is safe to promote the function (for
 /// example, all callers are direct).  If safe to promote some arguments, it
@@ -820,13 +824,11 @@ static Function *promoteArguments(Function *F, FunctionAnalysisManager &FAM,
     return nullptr;
 
   // First check: see if there are any pointer arguments!  If not, quick exit.
-  SmallVector<unsigned, 16> PointerArgNos;
-  for (unsigned I = 0; I < F->arg_size(); ++I) {
-    Argument *Arg = F->getArg(I);
-    if (Arg->getType()->isPointerTy())
-      PointerArgNos.push_back(I);
-  }
-  if (PointerArgNos.empty())
+  SmallVector<Argument *, 16> PointerArgs;
+  for (Argument &I : F->args())
+    if (I.getType()->isPointerTy())
+      PointerArgs.push_back(&I);
+  if (PointerArgs.empty())
     return nullptr;
 
   // Second check: make sure that all callers are direct callers.  We can't
@@ -861,8 +863,7 @@ static Function *promoteArguments(Function *F, FunctionAnalysisManager &FAM,
   // add it to ArgsToPromote.
   DenseMap<Argument *, SmallVector<OffsetAndArgPart, 4>> ArgsToPromote;
   unsigned NumArgsAfterPromote = F->getFunctionType()->getNumParams();
-  for (const auto &ArgIdx : PointerArgNos) {
-    Argument *PtrArg = F->getArg(ArgIdx);
+  for (Argument *PtrArg : PointerArgs) {
     // Replace sret attribute with noalias. This reduces register pressure by
     // avoiding a register copy.
     if (PtrArg->hasStructRetAttr()) {
@@ -876,15 +877,11 @@ static Function *promoteArguments(Function *F, FunctionAnalysisManager &FAM,
       }
     }
 
-    // Check if we can determine ahead of time that the argument is never
-    // modified by a call to this function.
-    bool ArgNotModified = callDoesNotModifyArg(F, ArgIdx, FAM);
-
     // If we can promote the pointer to its value.
     SmallVector<OffsetAndArgPart, 4> ArgParts;
 
     if (findArgParts(PtrArg, DL, AAR, MaxElements, IsRecursive, ArgParts,
-                     ArgNotModified)) {
+                     FAM)) {
       SmallVector<Type *, 4> Types;
       for (const auto &Pair : ArgParts)
         Types.push_back(Pair.second.Ty);

llvm/test/Transforms/ArgumentPromotion/actual-arguments.ll

@@ -68,8 +68,6 @@ define internal i32 @test_cannot_promote_3(ptr %p, ptr nocapture readonly %test_
   ret i32 %sum
 }
 
-; FIXME: We should perform ArgPromotion here!
-;
 ; This is called only by @caller_safe_args_1, from which we can prove that
 ; %test_c does not alias %p for any Call to the function, so we can promote it.
 ;
@@ -89,8 +87,6 @@ define internal i32 @test_can_promote_1(ptr %p, ptr nocapture readonly %test_c)
   ret i32 %sum
 }
 
-; FIXME: We should perform ArgPromotion here!
-;
 ; This is called by multiple callers (@caller_safe_args_1, @caller_safe_args_2),
 ; from which we can prove that %test_c does not alias %p for any Call to the
 ; function, so we can promote it.