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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson marked 3 inline comments as done.
dneilson added a comment.

In https://reviews.llvm.org/D38419#907201, @sanjoy wrote:

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


The current isa<> hierarchy is this, minus the transitive edges:

  digraph Existing {
    MemCpy -> MemTransfer
    MemMove -> MemTransfer
    MemTransfer -> MemI
    MemSet -> MemI
  }

So, all of the following are true: isa<MemTransfer>(MemCpy), isa<MemTransfer>(MemMove), isa<MemI>(MemTransfer), and isa<MemI>(MemSet). Plus, the transitive relations: isa<MemI>(MemCpy), isa<MemI>(MemMove)

The new hierarchy is this exact same thing within each of of the atomicity-types (non-atomic, Atomic, and Any). In addition we have as true:  isa<AnyX>(X) , isa<AnyX>(AtomicX), and all of the transitive edges like isa<AnyMemTransfer>(MemCpy) and isa<AnyMemI>(AtomicMemMove).

Stating the obvious...  this is not a bidirectional relation. For example, it is not the case that isa<X>(AnyX) or isa<AtomicX>(AnyX).



================
Comment at: include/llvm/IR/IntrinsicInst.h:218
+  /// common methods.
+  class MemIntrinsicBase : public IntrinsicInst {
   private:
----------------
sanjoy wrote:
> 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).
> ```
> 
I don't know that it actually matters whether they have a common C++ base class or not; I can't really see the base class ever being used outside of these class definitions.

CRTP seems to be more the norm in the LLVM codebase, so I could change this to use CRTP if only to be more consistent with other parts of LLVM.

I've no strong preference; I could go either way.


================
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.
----------------
sanjoy wrote:
> Maybe explicitly note that this also strips `addrspacecast`?
No harm in that.


================
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)));
     }
----------------
sanjoy wrote:
> Should we implement this for the atomic intrinsics as well?
The alignment stuff is trickier for the atomic memory intrinsics. Alignment is a property of the source & dest arguments, rather than an argument of its own. For the atomic intrinsics we'd need to implement getSourceAlignmentCst() and getDestAlignmentCst().

So, I was erring on the side of leaving it unimplemented for now and adding it in once we see whether it's useful and how it would be used.

Also, I'm hoping to get to revisiting the old patch that removes the alignment parameter from the non-atomic intrinsics and makes it an argument property. Once that's been done, then we could unify the alignment interfaces between atomic & non-atomic intrinsics.


================
Comment at: include/llvm/IR/IntrinsicInst.h:531
+      else
+        return cast<AtomicMemIntrinsic>(this)->isVolatile();
+    }
----------------
sanjoy wrote:
> 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.
Definitely considered it. I went for symmetry of implementation for no other reason than aesthetics.

Design-wise, there's definitely benefit to not having isVolatile() in the atomic hierarchy. Minimally, it's less confusing to implementers if it's not there -- since its always false for the atomic intrinsics.

If someone wants to add a volatile version of the atomic memory intrinsics down the line, for some reason, then they could always add isVolatile as part of that work.


https://reviews.llvm.org/D38419