Add two helper functions: isAtLeastAcquire, isAtLeastRelease

[morisset]

Summary

These methods are available to loads/stores respectively, and will be used
in a later separate patch.
This is patch 2/3 of the work started in D4797, but should be completely independent.

[jfb]

Shouldn't both also accept acq_rel?

[morisset]

If they were for generic instructions, yes (but generic instructions don't have a MemoryOrdering).
Here they are for loads/stores, which cannot have the acq_rel memory ordering (as said in a comment a few lines before each of my changes).
If this patch is later extended to RMW instructions, then yes acq_rel will have to be taken into account.

[jfb]

In D4844#5, @morisset wrote:

If they were for generic instructions, yes (but generic instructions don't have a MemoryOrdering).
Here they are for loads/stores, which cannot have the acq_rel memory ordering (as said in a comment a few lines before each of my changes).
If this patch is later extended to RMW instructions, then yes acq_rel will have to be taken into account.

Oh right. Could you add a short comment in each function body?

[morisset]

Add a comment to the body of the two functions, based on the comment of jfb

[jfb]

This lgtm, I'll leave it open for a while for others to comment, then I'll commit.

[reames]

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.

include/llvm/IR/Instructions.h

257

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..

258

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.

377

Same comments as above.

[morisset]

I moved the two helpers from being methods of LoadInst/StoreInst to being
free-standing functions operating on AtomicOrdering, on the suggestion of Philip Reames,
it is indeed much clearer.
Also added a doxygen comment (although it does not tell much more than the code..)
I did not change the name of the function, I hope it is more self-explanatory now that
they have an AtomicOrdering in their signature.
Using them elsewhere in the codebase will come in another patch if this one lands.

[reames]

With minor nitpicks, LGTM.

include/llvm/IR/Instructions.h

53

You should drop the anon namespace here.

57

Stylistic:
"( A ||

B)"

Neither is "better", but this is the norm in LLVM code. Please follow it.

[morisset]

Stylistic fixes to isAtLeastAcquire/isAtLeastRelease

[morisset]

commited in r215779.

include/llvm/IR/Instructions.h

enum AtomicOrdering {
   SequentiallyConsistent = 7
 };
 
 enum SynchronizationScope {
   SingleThread = 0,
   CrossThread = 1
 };
 
+namespace {
+/// Returns true if the ordering is at least as strong as acquire
+/// (i.e. acquire, acq_rel or seq_cst)
+inline bool isAtLeastAcquire(AtomicOrdering Ord) {
+   return (Ord == Acquire
+    || Ord == AcquireRelease
+    || Ord == SequentiallyConsistent);
+}
+
+/// Returns true if the ordering is at least as strong as release
+/// (i.e. release, acq_rel or seq_cst)
+inline bool isAtLeastRelease(AtomicOrdering Ord) {
+return (Ord == Release
+    || Ord == AcquireRelease
+    || Ord == SequentiallyConsistent);
+}
+} // end anonymous namespace
+
 //===----------------------------------------------------------------------===//
 //                                AllocaInst Class
 //===----------------------------------------------------------------------===//
 
 /// AllocaInst - an instruction to allocate memory on the stack
 ///
 class AllocaInst : public UnaryInstruction {
 protected:
void setAtomic(AtomicOrdering Ordering,
     setOrdering(Ordering);
     setSynchScope(SynchScope);
   }
 
   bool isSimple() const { return !isAtomic() && !isVolatile(); }
   bool isUnordered() const {
     return getOrdering() <= Unordered && !isVolatile();
   }
 
   Value *getPointerOperand() { return getOperand(0); }
   const Value *getPointerOperand() const { return getOperand(0); }
   static unsigned getPointerOperandIndex() { return 0U; }
 
   /// \brief Returns the address space of the pointer operand.
   unsigned getPointerAddressSpace() const {
     return getPointerOperand()->getType()->getPointerAddressSpace();
   }
 
void setAtomic(AtomicOrdering Ordering,
     setOrdering(Ordering);
     setSynchScope(SynchScope);
   }
 
   bool isSimple() const { return !isAtomic() && !isVolatile(); }
   bool isUnordered() const {
     return getOrdering() <= Unordered && !isVolatile();
   }
 
   Value *getValueOperand() { return getOperand(0); }
   const Value *getValueOperand() const { return getOperand(0); }
 
   Value *getPointerOperand() { return getOperand(1); }
   const Value *getPointerOperand() const { return getOperand(1); }
   static unsigned getPointerOperandIndex() { return 1U; }
 
   /// \brief Returns the address space of the pointer operand.