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

[Daniel Neilson via llvm-dev]

In case anyone would like to chime in. Review of this is ongoing in: https://reviews.llvm.org/D38419


> On Aug 28, 2017, at 10:02 AM, Daniel Neilson via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
> 
> 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
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev