[FunctionAttrs] Infer WriteOnly Function Attribute

[homerdin]

Summary

These changes expand the FunctionAttr logic in order to mark functions as WriteOnly when appropriate. This is done through an additional bool variable and extended logic. Any feedback would be appreciated!

[jdoerfert]

This patch looks fine to me.

I inlined some minor comments. Also, you should add a dedicated test case or explain in the commit message why that isn't possible yet.

lib/Transforms/IPO/FunctionAttrs.cpp

177 ↗

(On Diff #152130)

[Style] I'm not a fan of comment lines in a conditional without braces but I can see that you only copied the style already used here.

208 ↗

(On Diff #152130)

[Typo] Two dots.

276 ↗

(On Diff #152130)

Maybe you want to place an assertion here (or prior to the loop) that ensures ReadsMemory and WritesMemory are never both set. After this point, it is implicitly assumed anyway (Writes && Reads would otherwise lead to a ReadOnly attribute). Another benefit of an explicit assumption would be that it allows you to simplify the two conditions later on.

283 ↗

(On Diff #152130)

[Typo] Leading space

[homerdin]

Fixed typos and added assertion

[jdoerfert]

Ping. Are you going to upstream this?

[hfinkel]

A couple of minor comments, but otherwise, LGTM.

lib/Transforms/IPO/FunctionAttrs.cpp

209 ↗

(On Diff #157359)

I'd write this like the line below:

WritesMemory |= I->mayWriteToMemory();

286 ↗

(On Diff #157359)

Add space after if.

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

 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_MayWrite = 2,
+  MAK_WriteOnly = 3
 };
 
 /// Returns the memory access properties of this copy of the function.
 MemoryAccessKind computeFunctionBodyMemoryAccess(Function &F, AAResults &AAR);
 
 /// Computes function attributes in post-order over the call graph.
 ///
 /// By operating in post-order, this pass computes precise attributes for

llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp

 #include <vector>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "functionattrs"
 
 STATISTIC(NumReadNone, "Number of functions marked readnone");
 STATISTIC(NumReadOnly, "Number of functions marked readonly");
+STATISTIC(NumWriteOnly, "Number of functions marked writeonly");
 STATISTIC(NumNoCapture, "Number of arguments marked nocapture");
 STATISTIC(NumReturned, "Number of arguments marked returned");
 STATISTIC(NumReadNoneArg, "Number of arguments marked readnone");
 STATISTIC(NumReadOnlyArg, "Number of arguments marked readonly");
 STATISTIC(NumNoAlias, "Number of function returns marked noalias");
 STATISTIC(NumNonNullReturn, "Number of function returns marked nonnull");
 STATISTIC(NumNoRecurse, "Number of functions marked as norecurse");
 STATISTIC(NumNoUnwind, "Number of functions marked as nounwind");
static MemoryAccessKind checkFunctionMemoryAccess(Function &F, bool ThisBody,
   if (MRB == FMRB_DoesNotAccessMemory)
     // Already perfect!
     return MAK_ReadNone;
 
   if (!ThisBody) {
     if (AliasAnalysis::onlyReadsMemory(MRB))
       return MAK_ReadOnly;
 
-    // Conservatively assume it writes to memory.
+    if (AliasAnalysis::doesNotReadMemory(MRB))
+      return MAK_WriteOnly;
+
+    // Conservatively assume it reads and writes to memory.
     return MAK_MayWrite;
   }
 
   // Scan the function body for instructions that may read or write memory.
   bool ReadsMemory = false;
+  bool WritesMemory = false;
   for (inst_iterator II = inst_begin(F), E = inst_end(F); II != E; ++II) {
     Instruction *I = &*II;
 
     // Some instructions can be ignored even if they read or write memory.
     // Detect these now, skipping to the next instruction if one is found.
     CallSite CS(cast<Value>(I));
     if (CS) {
       // Ignore calls to functions in the same SCC, as long as the call sites
       // don't have operand bundles.  Calls with operand bundles are allowed to
       // have memory effects not described by the memory effects of the call
       // target.
       if (!CS.hasOperandBundles() && CS.getCalledFunction() &&
           SCCNodes.count(CS.getCalledFunction()))
         continue;
       FunctionModRefBehavior MRB = AAR.getModRefBehavior(CS);
       ModRefInfo MRI = createModRefInfo(MRB);
 
       // If the call doesn't access memory, we're done.
       if (isNoModRef(MRI))
         continue;
 
       if (!AliasAnalysis::onlyAccessesArgPointees(MRB)) {
-        // The call could access any memory. If that includes writes, give up.
+        // The call could access any memory. If that includes writes, note it.
         if (isModSet(MRI))
-          return MAK_MayWrite;
+          WritesMemory = true;
         // If it reads, note it.
         if (isRefSet(MRI))
           ReadsMemory = true;
         continue;
       }
 
       // Check whether all pointer arguments point to local memory, and
       // ignore calls that only access local memory.
       for (CallSite::arg_iterator CI = CS.arg_begin(), CE = CS.arg_end();
            CI != CE; ++CI) {
         Value *Arg = *CI;
         if (!Arg->getType()->isPtrOrPtrVectorTy())
           continue;
 
         AAMDNodes AAInfo;
         I->getAAMetadata(AAInfo);
         MemoryLocation Loc(Arg, MemoryLocation::UnknownSize, AAInfo);
 
         // 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.  Give up.
-          return MAK_MayWrite;
+          // Writes non-local memory.
+          WritesMemory = true;
         if (isRefSet(MRI))
           // Ok, it reads non-local memory.
           ReadsMemory = true;
       }
       continue;
     } else if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
       // Ignore non-volatile loads from local memory. (Atomic is okay here.)
       if (!LI->isVolatile()) {
if (CS) {
       // Ignore vaargs on local memory.
       MemoryLocation Loc = MemoryLocation::get(VI);
       if (AAR.pointsToConstantMemory(Loc, /*OrLocal=*/true))
         continue;
     }
 
     // Any remaining instructions need to be taken seriously!  Check if they
     // read or write memory.
-    if (I->mayWriteToMemory())
-      // Writes memory.  Just give up.
-      return MAK_MayWrite;
+    //
+    // Writes memory, remember that.
+    WritesMemory |= I->mayWriteToMemory();
 
     // If this instruction may read memory, remember that.
     ReadsMemory |= I->mayReadFromMemory();
   }
 
+  if (WritesMemory) { 
+    if (!ReadsMemory)
+      return MAK_WriteOnly;
+    else
+      return MAK_MayWrite;
+  }
+
   return ReadsMemory ? MAK_ReadOnly : MAK_ReadNone;
 }
 
 MemoryAccessKind llvm::computeFunctionBodyMemoryAccess(Function &F,
                                                        AAResults &AAR) {
   return checkFunctionMemoryAccess(F, /*ThisBody=*/true, AAR, {});
 }
 
 /// Deduce readonly/readnone attributes for the SCC.
 template <typename AARGetterT>
 static bool addReadAttrs(const SCCNodeSet &SCCNodes, AARGetterT &&AARGetter) {
   // 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:
       return false;
     case MAK_ReadOnly:
       ReadsMemory = true;
       break;
+    case MAK_WriteOnly:
+      WritesMemory = true;
+      break;
     case MAK_ReadNone:
       // Nothing to do!
       break;
     }
   }
 
   // Success!  Functions in this SCC do not access memory, or only read memory.
   // Give them the appropriate attribute.
   bool MadeChange = false;
+
+  assert(!(ReadsMemory && WritesMemory) &&
+          "Function marked read-only and write-only");
   for (Function *F : SCCNodes) {
     if (F->doesNotAccessMemory())
       // Already perfect!
       continue;
 
     if (F->onlyReadsMemory() && ReadsMemory)
       // No change.
       continue;
 
+    if (F->doesNotReadMemory() && WritesMemory)
+      continue;
+
     MadeChange = true;
 
     // Clear out any existing attributes.
     F->removeFnAttr(Attribute::ReadOnly);
     F->removeFnAttr(Attribute::ReadNone);
+    F->removeFnAttr(Attribute::WriteOnly);
 
     // Add in the new attribute.
+    if (WritesMemory && !ReadsMemory)
+      F->addFnAttr(Attribute::WriteOnly);
+    else
       F->addFnAttr(ReadsMemory ? Attribute::ReadOnly : Attribute::ReadNone);
 
-    if (ReadsMemory)
+    if (WritesMemory && !ReadsMemory)
+      ++NumWriteOnly;
+    else if (ReadsMemory)
       ++NumReadOnly;
     else
       ++NumReadNone;
   }
 
   return MadeChange;
 }
 

llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll

 ; Similar to the others, va_arg only accesses memory through its operand.
 
 ; CHECK: define i32 @test3_yes(i8* nocapture %p) #0 {
 define i32 @test3_yes(i8* %p) nounwind {
   %t = va_arg i8* %p, i32, !tbaa !1
   ret i32 %t
 }
 
-; CHECK: define i32 @test3_no(i8* nocapture %p) #1 {
+; CHECK: define i32 @test3_no(i8* nocapture %p) #5 {
 define i32 @test3_no(i8* %p) nounwind {
   %t = va_arg i8* %p, i32, !tbaa !2
   ret i32 %t
 }
 
 declare void @callee(i32* %p) nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i1) nounwind
 
 ; CHECK: attributes #0 = { norecurse nounwind readnone }
-; CHECK: attributes #1 = { norecurse nounwind }
+; CHECK: attributes #1 = { norecurse nounwind writeonly }
 ; CHECK: attributes #2 = { nounwind readonly }
 ; CHECK: attributes #3 = { nounwind }
 ; CHECK: attributes #4 = { nounwind readnone }
-; CHECK: attributes #5 = { argmemonly nounwind }
+; CHECK: attributes #5 = { norecurse nounwind }
+; CHECK: attributes #6 = { argmemonly nounwind }
 
 ; Root note.
 !0 = !{ }
 
 ; Invariant memory.
 !1 = !{!3, !3, i64 0, i1 1 }
 ; Not invariant memory.
 !2 = !{!3, !3, i64 0, i1 0 }
 !3 = !{ !"foo", !0 }