[PATCH] D18714: Add writeonly IR attribute

Nicolai Hähnle via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 27 11:51:03 PDT 2016


nhaehnle marked 4 inline comments as done.
nhaehnle added a comment.

Hi Philip, thanks for the feedback. All comments are either reflected in the updated revisions or answered here.


================
Comment at: include/llvm/IR/Attributes.td:163
@@ -162,1 +162,3 @@
 
+/// Function only writes to memory.
+def WriteOnly : EnumAttr<"writeonly">;
----------------
reames wrote:
> I'm pretty sure that order doesn't matter in this file.  Given that, please put this near readonly.  
I also believe the order doesn't matter, but I didn't do that because everything else is _very_ thoroughly alphabetized. Note how ArgMemOnly and InaccessibleMem(OrArgMem)Only also aren't close together...

================
Comment at: lib/IR/Attributes.cpp:272
@@ -271,1 +271,3 @@
     return "uwtable";
+  if (hasAttribute(Attribute::WriteOnly))
+    return "writeonly";
----------------
reames wrote:
> Here and a couple other places in C++ code: not order sensitive, please group with other memory attributes.
Done here, since there's already a precedent for non-alphabetization, but not in getAttrMask where there is a strict order by bit number...

================
Comment at: lib/IR/Verifier.cpp:1498
@@ +1497,3 @@
+
+  Assert(
+      !(Attrs.hasAttribute(AttributeSet::FunctionIndex, Attribute::ReadOnly) &&
----------------
reames wrote:
> Duplicate code?
readnone/writeonly vs. readonly/writeonly, so no duplication as far as I can see. The whole sequence is full of similarities, but this is probably not the right place to clean it up.


http://reviews.llvm.org/D18714





More information about the llvm-commits mailing list