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

[Sanjoy Das via llvm-dev]

Hi Daniel,

On Thu, Aug 17, 2017 at 8:32 AM, Daniel Neilson via llvm-dev
<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?

-- Sanjoy