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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm

I'd suggest waiting 1-2 days to give time for someone to chime in (after reading the llvm-dev ping you sent out) and then checking this in.



================
Comment at: include/llvm/IR/IntrinsicInst.h:218
+  /// common methods.
+  class MemIntrinsicBase : public IntrinsicInst {
   private:
----------------
dneilson wrote:
> 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.
> 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.

Another way of stating what I meant to say is that the fact that there is a `MemIntrinsicBase` class that is the base of all these classes is an implementation detail (I assumed this, maybe you have a different picture in mind); and for this reason we should help users not rely on this property by not actually providing a common base class.


================
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)));
     }
----------------
dneilson wrote:
> 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.
SGTM


================
Comment at: include/llvm/IR/IntrinsicInst.h:531
+      else
+        return cast<AtomicMemIntrinsic>(this)->isVolatile();
+    }
----------------
dneilson wrote:
> 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.
Minor nit: you could just use one `dyn_cast` here:

```
if (auto *MI = dyn_cast<MemIntrinsic>(...))
  return MI->isVolatile();
```



================
Comment at: include/llvm/IR/IntrinsicInst.h:217
+  /// Common base class for all memory intrinsics. Simply provides
+  /// common methods.
+  template<typename Derived>
----------------
Please also add a comment on why we have CRTP here.


================
Comment at: lib/IR/Verifier.cpp:4034
+        cast<AtomicMemCpyInst>(CS.getInstruction());
     ;
 
----------------
Mind removing this extra semi-colon as well?


https://reviews.llvm.org/D38419