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

Hi Sanjoy,
 Response/thoughts below...


On Aug 19, 2017, at 3:13 PM, Sanjoy Das <sanjoy at
playingwithpointers.com<mailto:sanjoy at playingwithpointers.com>>
wrote:

Hi Daniel,

On Thu, Aug 17, 2017 at 8:32 AM, Daniel Neilson via llvm-dev
<llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>>
wrote:
Cons:
One more attribute to check when implementing a pass that handles a memory
intrinsic; could lead to some unexpected bugs where we change an
unordered-atomic intrinsic into a regular intrinsic.

The con here is pretty concerning. We risk making it easy for a developer
to forget about the element-atomic variants when introducing an optimization
that converts a memmove/memcpy/memset. Such an optimization could,
conceivably, result in an unordered-atomic intrinsic being erroneously
converted to drop its atomicity; such a bug would only exhibit as a race
condition in the resulting program, and could be an absolute beast to debug.
The only way to prevent such bugs would be very careful code reviews.

The con is compounded by the fact that since clang will not (?)
generate atomic memX instructions, it will be possible to validate a
change to LLVM across an arbitrarily large corpus of C and C++
programs and not notice this sort of bug.  In theory this isn't a
problem (since code reviews can catch bugs like this), but IMO there
is non-trivial value in avoiding a situation that allows bug in the
Java frontends that don't manifest in clang.

Option 2)
-------------
Add separate classes to the hierarchy for each intrinsic. There are a few
different ways that this could be organized (note: UA* = unordered-atomic
version of intrinsic, to save typing):

a) Completely disjoint hierarchy
UAMemIntrinsic
* UAMemSetInst
* UAMemTransferInst
** UAMemCpyInst
** UAMemMoveInst
MemIntrinsic
* MemSetInst
* MemTransferInst
** MemCpyInst
** MemMoveInst

e) Multiple inheritance

This is a variant on options 2b to 2d. To make it possible to easily use
isa<> to query for whether or not a memory intrinsic is unordered-atomic
or
not we could add an interface-like class that all unordered-atomic memory
intrinsic classes would multiply inherit from. I’m not sure if there is
precedence for this sort of thing in the instruction class hierarchy,
though; I haven’t reviewed the entire hierarchy, but all of the parts that I
have looked at appear to be single inheritance.

There is precedent for something like this:  OverflowingBinaryOperator
will return true for both isa<Instruction> and isa<ConstantExpr>.  I
also think this combined with (a) is probably the cleanest path
forward:

AtomicMemIntrinsic
* AtomicMemSetInst
* AtomicMemTransferInst
** AtomicMemCpyInst
** AtomicMemMoveInst
PlainMemIntrinsic
* PlainMemSetInst
* PlainMemTransferInst
** PlainMemCpyInst
** PlainMemMoveInst

MemIntrinsic : PlainMemIntrinsic, AtomicMemIntrinsic
MemSetInst : PlainMemSetInst, AtomicMemSetInst
MemTransferInst : PlainMemTransferInst, AtomicMemTransferInst
MemCpyInst : PlainMemCpyInst, AtomicMemCpyInst
MemMoveInst : PlainMemMoveInst, AtomicMemMoveInst

with the existing uses of MemIntrinsic changed to PlainMemIntrinsic etc.

What do you think?

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.

 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.

 That would leave the meanings of the current MemIntrinsic classes completely
unchanged; which should be good for 3rd parties. Updating a pass to work with
unordered-atomic memory intrinsics would just mean having it use the AnyMem*
classes instead of the Mem* classes, and then adding some isa<> checks to
do the right thing with the atomic variants. We don’t get all of the memory
intrinsic optimizing passes for free on the atomic variants, but we get the ones
we want at a reasonably low cost, and there’s no risk to the behaviour of the
existing non-atomic memory intrinsics...

-Daniel

---
Daniel Neilson, Ph.D.
Azul Systems

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170821/1f09c413/attachment.html>

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

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