[PATCH] D18714: Add writeonly IR attribute

[Philip Reames via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

A bunch of minor comments.  Seems close to going in.  I'd have preferred you split the addition of the attribute from the intrinsic changes, but will not require you to split them now.


================
Comment at: docs/LangRef.rst:1385
@@ +1384,3 @@
+``writeonly``
+    On a function, this attribute indicates that the function writes to but
+    does not read from memory.
----------------
may write to

================
Comment at: include/llvm/IR/Attributes.td:163
@@ -162,1 +162,3 @@
 
+/// Function only writes to memory.
+def WriteOnly : EnumAttr<"writeonly">;
----------------
I'm pretty sure that order doesn't matter in this file.  Given that, please put this near readonly.  

================
Comment at: include/llvm/IR/Intrinsics.td:378
@@ -371,3 +377,3 @@
                              llvm_i32_ty, llvm_i1_ty],
-                            [IntrReadWriteArgMem, NoCapture<0>]>;
+                            [IntrWriteArgMem, NoCapture<0>]>;
 
----------------
WriteOnly<0>, IntrArgMemOnly would be slightly more consistent.  

================
Comment at: lib/Analysis/AliasAnalysis.cpp:146
@@ -145,1 +145,3 @@
 
+  if (doesNotReadMemory(MRB))
+    Result = ModRefInfo(Result & MRI_Mod);
----------------
minor: this is exclusive of the previous case.  using an else if to indicate this here and all other similar cases would be mildly useful.  

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:627
@@ -626,3 +617,1 @@
 
-  // We can bound the aliasing properties of memset_pattern16 just as we can
-  // for memcpy/memset.  This is particularly important because the
----------------
Please leave this comment.  

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:633
@@ -645,4 +632,3 @@
 
-  // Emulate the missing writeonly attribute by checking for known builtin
-  // intrinsics and target library functions.
+  // Checking for known target library functions.
   if (isWriteOnlyParam(CS, ArgIdx, TLI))
----------------
Don't drop the intrinsic bit from the comment.

================
Comment at: lib/IR/Attributes.cpp:272
@@ -271,1 +271,3 @@
     return "uwtable";
+  if (hasAttribute(Attribute::WriteOnly))
+    return "writeonly";
----------------
Here and a couple other places in C++ code: not order sensitive, please group with other memory attributes.

================
Comment at: lib/IR/Verifier.cpp:1498
@@ +1497,3 @@
+
+  Assert(
+      !(Attrs.hasAttribute(AttributeSet::FunctionIndex, Attribute::ReadOnly) &&
----------------
Duplicate code?


http://reviews.llvm.org/D18714