[PATCH] D19291: Split IntrReadArgMem into IntrReadMem and IntrArgMemOnly

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM.  I have tried to review all the changes, but it's possible I missed something in the mechanical parts.  I'm assuming author did this via find/replace or other tooling so that chances of typos are low.

The naming and style comments would be best addressed in a separate commit.


================
Comment at: include/llvm/IR/Intrinsics.td:34
@@ +33,3 @@
+// memory and has no other side effects. Therefore, it cannot be moved across
+// stores (unless also marked as IntrArgMemOnly). However, it can be reordered
+// otherwise and can be deleted if dead.
----------------
The (unless also...) part is confusing.  Possibly, "it cannot be moved across aliasing potentially stores"?

================
Comment at: utils/TableGen/CodeGenTarget.cpp:451
@@ -450,3 +450,3 @@
   std::string DefName = R->getName();
   ModRef = ReadWriteMem;
   isOverloaded = false;
----------------
For clarify, should this become ReadWriteAllMem?

================
Comment at: utils/TableGen/CodeGenTarget.cpp:577
@@ -578,3 +576,3 @@
     else if (Property->getName() == "IntrReadMem")
-      ModRef = ReadMem;
+      ModRef = ModRefBehavior(ModRef & ReadMem);
     else if (Property->getName() == "IntrWriteMem")
----------------
This appears to be correct, but slightly confusing.  I don't see an obvious way to make it better other than possibly switching from using ModRefBehavior names to explicitly ORd bits.  Eg.

Start with ModRef = RWA;
...
else if (Property->getName() == "IntrWriteMem")
      ModRef = ModRefBehavior(ModRef & ~MR_Ref);


http://reviews.llvm.org/D19291