Add isElementAtomic query method to MemInstrinsic class.

[dneilson]

Summary

We are working towards being able to insert the element unordered atomic memcpy (see: https://reviews.llvm.org/D33240) to the introspection hierarchy as a subclass of MemCpyInst.

This NFC is a first step that introduces a method to the MemIntrinsic class that can be used to query whether, or not, a given memory intrinsic is element atomic; this is being added to MemIntrinsic, rather than MemCpyInst, because the intention is to introduce element atomic memset & memmove intrinsics in the near future as well.

The plan is that future patches will, one at a time, update passes and introduce test cases to ensure that isElementAtomic() of a MemIntrinsic is properly handled -- ensuring fall-through to current behaviour when isElementAtomic() is false, and at least a graceful exit when it returns true. Once that is complete a final change that adds the element atomic memcpy intrinsic as a subclass of MemCpyInst will be proposed that also updates tests to show that passes do the right thing with the element atomic memcpy.

[efriedma]

Could you include one of the users of this function in the patch so it's clear how you're intending to use it?

I think this looks okay.

[mkazantsev]

include/llvm/IR/IntrinsicInst.h

360

Would you mind to give a more detailed description of what "element atomic" actually is in this comment? Maybe write some simple example of array and show how it works with its elements. I'd ask this to make clear distinction between this one and isAtomic() method.

[dneilson]

In D34629#791320, @efriedma wrote:

Could you include one of the users of this function in the patch so it's clear how you're intending to use it?

I think this looks okay.

@efriedma Please see: https://reviews.llvm.org/D34883 I would also appreciate your feedback on the process outlined in that change.

[efriedma]

Oh, I see...

What's the advantage of "I->isElementAtomic()" over "isa<ElementUnorderedAtomicMemCpyInst>(I)"?

Not sure it makes sense to make ElementUnorderedAtomicMemCpyInst inherit from MemCpyInst; it's likely to be confusing that a MemCpyInst might not really be a memcpy.

[dneilson]

In D34629#797105, @efriedma wrote:

Oh, I see...

What's the advantage of "I->isElementAtomic()" over "isa<ElementUnorderedAtomicMemCpyInst>(I)"?

Not sure it makes sense to make ElementUnorderedAtomicMemCpyInst inherit from MemCpyInst; it's likely to be confusing that a MemCpyInst might not really be a memcpy.

isa<> is possible, but then we'd probably have to have something like:

• MemIntrinsic
  • MemSetInst
  • MemTransferInst
    • MemCpyInst
    • MemMoveInst
  • ElementAtomicMemIntrinsic
    • ElementAtomicMemSetInst
    • ElementAtomicMemTransferInst
      • ElementAtomicMemCpyInst
      • ElementAtomicMemMoveInst

Would be more work updating passes, and will very likely result in a fair bit of code cloning. ex: When a pass's treatment of ElementAtomicMemTransferInst is identical to its treatment of MemTransferInst, and it's using a visitor to go through all instructions in a function/block/loop/etc. But, in exchange, we'd get a clear separation of concerns. Pros & cons to both options, I think.

Opinions?

[dneilson]

Closing. Discussion & path forward will be decided in the RFC discussion hanging off of http://lists.llvm.org/pipermail/llvm-dev/2017-August/116589.html

include/llvm/IR/IntrinsicInst.h

public:
     void setVolatile(Constant* V) {
       setArgOperand(4, V);
     }
 
     Type *getAlignmentType() const {
       return getArgOperand(3)->getType();
     }
 
+    // Query for whether the instruction is one of the unordered element
+    // atomic variants of memory intrinsic.
+    bool isElementAtomic() const { return false; }
+
     // Methods for support type inquiry through isa, cast, and dyn_cast:
     static inline bool classof(const IntrinsicInst *I) {
       switch (I->getIntrinsicID()) {
       case Intrinsic::memcpy:
       case Intrinsic::memmove:
       case Intrinsic::memset:
         return true;
       default: return false;