[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