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

[Daniel Neilson via llvm-dev]

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>