<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
<div class=""><br class="">
</div>
<div class="">Hi Sanjoy,</div>
<div class=""> Response/thoughts below...</div>
<br class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
<div style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<br class="">
</div>
</div>
</div>
<div>
<blockquote type="cite" class="">
<div class="">On Aug 19, 2017, at 3:13 PM, Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com" class="">sanjoy@playingwithpointers.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">Hi Daniel,<br class="">
<br class="">
On Thu, Aug 17, 2017 at 8:32 AM, Daniel Neilson via llvm-dev<br class="">
<<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>> wrote:<br class="">
<blockquote type="cite" class="">Cons:<br class="">
One more attribute to check when implementing a pass that handles a memory<br class="">
intrinsic; could lead to some unexpected bugs where we change an<br class="">
unordered-atomic intrinsic into a regular intrinsic.<br class="">
<br class="">
The con here is pretty concerning. We risk making it easy for a developer<br class="">
to forget about the element-atomic variants when introducing an optimization<br class="">
that converts a memmove/memcpy/memset. Such an optimization could,<br class="">
conceivably, result in an unordered-atomic intrinsic being erroneously<br class="">
converted to drop its atomicity; such a bug would only exhibit as a race<br class="">
condition in the resulting program, and could be an absolute beast to debug.<br class="">
The only way to prevent such bugs would be very careful code reviews.<br class="">
</blockquote>
<br class="">
The con is compounded by the fact that since clang will not (?)<br class="">
generate atomic memX instructions, it will be possible to validate a<br class="">
change to LLVM across an arbitrarily large corpus of C and C++<br class="">
programs and not notice this sort of bug. In theory this isn't a<br class="">
problem (since code reviews can catch bugs like this), but IMO there<br class="">
is non-trivial value in avoiding a situation that allows bug in the<br class="">
Java frontends that don't manifest in clang.<br class="">
<br class="">
<blockquote type="cite" class="">Option 2)<br class="">
-------------<br class="">
Add separate classes to the hierarchy for each intrinsic. There are a few<br class="">
different ways that this could be organized (note: UA* = unordered-atomic<br class="">
version of intrinsic, to save typing):<br class="">
<br class="">
a) Completely disjoint hierarchy<br class="">
UAMemIntrinsic<br class="">
* UAMemSetInst<br class="">
* UAMemTransferInst<br class="">
** UAMemCpyInst<br class="">
** UAMemMoveInst<br class="">
MemIntrinsic<br class="">
* MemSetInst<br class="">
* MemTransferInst<br class="">
** MemCpyInst<br class="">
** MemMoveInst<br class="">
<br class="">
e) Multiple inheritance<br class="">
<br class="">
This is a variant on options 2b to 2d. To make it possible to easily use<br class="">
isa<> to query for whether or not a memory intrinsic is unordered-atomic or<br class="">
not we could add an interface-like class that all unordered-atomic memory<br class="">
intrinsic classes would multiply inherit from. I’m not sure if there is<br class="">
precedence for this sort of thing in the instruction class hierarchy,<br class="">
though; I haven’t reviewed the entire hierarchy, but all of the parts that I<br class="">
have looked at appear to be single inheritance.<br class="">
</blockquote>
<br class="">
There is precedent for something like this: OverflowingBinaryOperator<br class="">
will return true for both isa<Instruction> and isa<ConstantExpr>. I<br class="">
also think this combined with (a) is probably the cleanest path<br class="">
forward:<br class="">
<br class="">
AtomicMemIntrinsic<br class="">
* AtomicMemSetInst<br class="">
* AtomicMemTransferInst<br class="">
** AtomicMemCpyInst<br class="">
** AtomicMemMoveInst<br class="">
PlainMemIntrinsic<br class="">
* PlainMemSetInst<br class="">
* PlainMemTransferInst<br class="">
** PlainMemCpyInst<br class="">
** PlainMemMoveInst<br class="">
<br class="">
MemIntrinsic : PlainMemIntrinsic, AtomicMemIntrinsic<br class="">
MemSetInst : PlainMemSetInst, AtomicMemSetInst<br class="">
MemTransferInst : PlainMemTransferInst, AtomicMemTransferInst<br class="">
MemCpyInst : PlainMemCpyInst, AtomicMemCpyInst<br class="">
MemMoveInst : PlainMemMoveInst, AtomicMemMoveInst<br class="">
<br class="">
with the existing uses of MemIntrinsic changed to PlainMemIntrinsic etc.<br class="">
<br class="">
What do you think?<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
<div class="">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. </div>
<div class=""><br class="">
</div>
<div class="">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:</div>
<div class="">1) NFC to rename MemIntrinsic classes to PlainMemIntrinsic</div>
<div class="">2) Patch to add AtomicMemIntrinsic classes & MemIntrinsic classes derived from Plain* & Atomic*</div>
<div class="">3) Individual patches to update passes to handle Atomic + Plain as desired.</div>
<div class=""><br class="">
</div>
<div class=""> 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.</div>
<div class=""><br class="">
</div>
<div class=""> Thinking out loud… If we go the multiple inheritance route, then what about something like:</div>
<div class="">MemIntrinsic</div>
<div class="">* MemSetInst</div>
<div class="">* MemTransferInst</div>
<div class="">** MemCpyInst</div>
<div class="">** MemMoveInst</div>
<div class="">AtomicMemIntrinsic</div>
<div class="">* AtomicMemSetInst</div>
<div class="">* AtomicMemTransferInst</div>
<div class="">** AtomicMemCpyInst</div>
<div class="">** AtomicMemMoveInst</div>
<div class=""><br class="">
</div>
<div class="">AnyMemIntrinsic : MemIntrinsic, AtomicMemIntrinsic</div>
<div class="">AnyMemSetInst : MemSetInst, AtomicMemSetInst</div>
<div class="">AnyMemTransferInst : MemTranferInst, AtomicMemTransferInst</div>
<div class="">… etc.</div>
<div class=""><br class="">
</div>
<div class=""> 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...</div>
<div class=""><br class="">
</div>
<div class="">-Daniel</div>
<div class=""><br class="">
</div>
<div class="">
<div>---</div>
<div>Daniel Neilson, Ph.D.</div>
<div>Azul Systems</div>
<div class=""><br class="">
</div>
</div>
</body>
</html>