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

[Daniel Neilson via llvm-dev]

Hi Sanjoy,
 Sounds like a good plan to me. Unless someone wants to chime in with a better idea then I’ll try this approach, and see how it goes.

-Daniel

> On Aug 21, 2017, at 12:12 PM, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> 
> 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