[PATCH] Add two helper functions: isAtLeastAcquire, isAtLeastRelease

Philip Reames listmail at philipreames.com
Tue Aug 12 12:16:18 PDT 2014


Generally, we prefer that functions like this are used in the codebase.  It's acceptable to have closely following patches, but I see a couple places you could use these today.  

for example: Arch64ISelLowering.cpp could make use of isAcquireOrdering.

As a high level structure, I'd suggest making these predicates on orderings, not predicates on instructions.  This would remove a lot of the complexity around which instructions legally have which orders.  This also seems to be how existing code is structured.

LGTM with inline comments addressed.  The structural comments above is optional.

================
Comment at: include/llvm/IR/Instructions.h:239
@@ -238,1 +238,3 @@
   }
+  bool isAtLeastAcquire() const {
+    // We do not have to check for AcquireRelease as this is a load and
----------------
Please add a doxygen comment describing what this does.  I might also pick a different name.  Most readers won't know that acquire refers to memory ordering.  Possible: "isAcquireOrdered", "isOrderedAcquireOrStronger", etc..

================
Comment at: include/llvm/IR/Instructions.h:240
@@ +239,3 @@
+  bool isAtLeastAcquire() const {
+    // We do not have to check for AcquireRelease as this is a load and
+    // it is not a valid ordering for loads.
----------------
I would prefer you simply checked for AcquireRelease.  While it might be dead code for a Load, it decrease the odds of a bug if this code is copied elsewhere.  Alternatively, turn the comment into an assert that the ordering is not AcquireRelease.

================
Comment at: include/llvm/IR/Instructions.h:365
@@ -358,1 +364,3 @@
   }
+  bool isAtLeastRelease() const {
+    // We do not have to check for AcquireRelease as this is a store and
----------------
Same comments as above.

http://reviews.llvm.org/D4844






More information about the llvm-commits mailing list