[llvm-dev] [RFC] Injecting new element atomic memory intrinsics into MemIntrinsic class hierarchy

[Sanjoy Das via llvm-dev]

Hi Daniel,

On Mon, Aug 21, 2017 at 7:57 AM, Daniel Neilson <dneilson at azul.com> wrote:
> It’s an interesting idea. I kind of like it — it’s an interesting mix of
> options 1, 2a, & 2e. An advantage over option 1 is that this also exposes an
> easy way for a pass to not even have to think about the atomic memory
> intrinsics — just key off of the PlainMemIntrinsic classes instead of the
> MemIntrinsic classes.
>
> We still have the same con as option 1, though I could virtually eliminate
> that con by starting the work by submitting an NFC that renames the current
> MemIntrinsic & derived class to PlainMemIntrinsic, etc. That would make the
> path forward something like:
> 1) NFC to rename MemIntrinsic classes to PlainMemIntrinsic
> 2) Patch to add AtomicMemIntrinsic classes & MemIntrinsic classes derived
> from Plain* & Atomic*
> 3) Individual patches to update passes to handle Atomic + Plain as desired.
>
>  The problem that I see with that, though, is any third party stuff that
> uses the MemIntrinsic classes as they are today will break between steps (1)
> and (2), and then they will start doing the wrong things with the
> unordered-atomic memory intrinsics if they’re presented with one — though,
> that last bit is no different from option 1.

My current understanding is that we don't have any actual here beyond
publicizing the changes on llvm-dev and not changing the world
overnight.  I think as long as you make change (1), send a heads-up
email to llvm-dev and make sure there is a gap of a day or two between
(1) and (2), we're fine.  LLVM does not have a stable C++ API.

Having said that:

>  Thinking out loud… If we go the multiple inheritance route, then what about
> something like:
> MemIntrinsic
> * MemSetInst
> * MemTransferInst
> ** MemCpyInst
> ** MemMoveInst
> AtomicMemIntrinsic
> * AtomicMemSetInst
> * AtomicMemTransferInst
> ** AtomicMemCpyInst
> ** AtomicMemMoveInst
>
> AnyMemIntrinsic : MemIntrinsic, AtomicMemIntrinsic
> AnyMemSetInst : MemSetInst, AtomicMemSetInst
> AnyMemTransferInst : MemTranferInst, AtomicMemTransferInst
> … etc.

I like the Any* prefix.  But how about:

AnyMemIntrinsic : PlainMemIntrinsic, AtomicMemIntrinsic
AnyMemSetInst : PlainMemSetInst, AtomicMemSetInst

etc. ?  This also will prevent silent failures in downstream projects.

-- Sanjoy