[PATCH] D38419: Create instruction classes for identifying any atomicity of memory intrinsic. (NFC)

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Just to be clear, at the `isa<>`, `dyn_cast<>` level, this is the hierarchy you're shooting for right:

  digraph G {
  	AnyMemI -> MemI
  	AnyMemI -> AtomicMemI
  	AnyMemI -> AnyMemSetI
  	AnyMemI -> AnyMemTransferI
  
  	AnyMemSetI -> MemSetI
  	AnyMemSetI -> AtomicMemSetI
  
  	AnyMemTransferI -> AtomicMemTransferI
  	AnyMemTransferI -> MemTransferI
  	AnyMemTransferI -> AnyMemCpyI
  	AnyMemTransferI -> AnyMemMoveI
  
  	AtomicMemTransferI -> AtomicMemcpyI
  	AtomicMemTransferI -> AtomicMemMoveI
  
  	MemTransferI -> MemcpyI
  	MemTransferI -> MemMoveI
  
  	AnyMemCpyI -> MemcpyI
  	AnyMemCpyI -> AtomicMemcpyI
  
  	AnyMemMoveI -> MemMoveI
  	AnyMemMoveI -> AtomicMemMoveI
  
  	MemI -> MemTransferI
  	MemI -> MemMoveI
  
  	AtomicMemI -> AtomicMemTransferI
  	AtomicMemI -> AtomicMemMoveI
  }



================
Comment at: include/llvm/IR/IntrinsicInst.h:218
+  /// common methods.
+  class MemIntrinsicBase : public IntrinsicInst {
   private:
----------------
This will let users up-cast `AtomicMemInstrinsic` and `MemIntrinsic` objects to `MemIntrinsicBase` objects -- is that okay?  If it isn't, we can use CRTP to avoid giving `AtomicMemInstrinsic` and `MemIntrinsic` the same base class, as in:

```
template<typename Derived>
class MemIntrinsicBase : public IntrinsicInst { ... /* No real use of Derived anywhere */ };

class AtomicMemIntrinsic : public MemIntrinsicBase<AtomicMemIntrinsic> { ... };
class MemIntrinsic : public MemIntrinsicBase<MemIntrinsic> { ... };

// Now MemIntrinsic and AtomicMemIntrinsic do not have a common base class (at the C++ level).
```



================
Comment at: include/llvm/IR/IntrinsicInst.h:236
     /// This is just like getRawDest, but it strips off any cast
     /// instructions that feed it, giving the original input.  The returned
     /// value is guaranteed to be a pointer.
----------------
Maybe explicitly note that this also strips `addrspacecast`?


================
Comment at: include/llvm/IR/IntrinsicInst.h:320
 
-    Value *getRawElementSizeInBytes() const {
-      return const_cast<Value *>(getArgOperand(ARG_ELEMENTSIZE));
+    static inline bool classof(const IntrinsicInst *I) {
+      return I->getIntrinsicID() == Intrinsic::memset_element_unordered_atomic;
----------------
The `inline` attribute is not necessary here I think -- member defined within the class are automatically `inline`.


================
Comment at: include/llvm/IR/IntrinsicInst.h:401
     ConstantInt *getAlignmentCst() const {
-      return cast<ConstantInt>(const_cast<Value*>(getArgOperand(3)));
+      return cast<ConstantInt>(const_cast<Value*>(getArgOperand(ARG_ALIGN)));
     }
----------------
Should we implement this for the atomic intrinsics as well?


================
Comment at: include/llvm/IR/IntrinsicInst.h:523
 
+  // The common base class for the atomic memset/memmove/memcpy intrinsics
+  // i.e. llvm.element.unordered.atomic.memset/memcpy/memmove
----------------
Isn't the the common base for atomic *and non-atomic* mem(set|move|cpy)?


================
Comment at: include/llvm/IR/IntrinsicInst.h:531
+      else
+        return cast<AtomicMemIntrinsic>(this)->isVolatile();
+    }
----------------
Did you consider not providing the `isVolatile()` accessor on `AtomicMemIntrinsic`?  Design-wise I find that (not having the acccessor) slightly more preferable, but if that would require a lot of extra code then it isn't worth it.


================
Comment at: include/llvm/IR/IntrinsicInst.h:630
+
+  /// This class represents the atomic memcpy intrinsic
+  /// i.e. llvm.element.unordered.atomic.memcpy
----------------
These comments need to be updated.


https://reviews.llvm.org/D38419