llvm dev - Aug 2014 - [LLVMdev] Plan to optimize atomics in LLVM

> From my reading of Atomics.rst, it would be sound to reorder (It does not
> say much about load-linked, so I am treating it as a normal load here)
>
>> store seq_cst
>> fence release
>> load-linked monotonic
>
> into
>
>> load-linked monotonic
>> store seq_cst
>> fence release
> Which would make an execution ending in %old_x = %old_y = 0 possible, while
> it is impossible in the original program.
Hmm, evil. Well, I'm convinced. Thanks very much for taking the time
to come up with an example and telling us about it.
> Fixing it is a two line change in insertLeadingFence, but it triggers some
> test failures, both because of tests looking specifically for a fence
> release here,
That's fine, we can change those easily enough. And the "dmb
ishst"
(as I understand it, it *is* a release fence, but not almost certainly
not suitable for preventing this reordering).

Cheers.

Tim.

Hi,

I have done a refactoring that merges AtomicExpandLoadLinkedPass and
X86AtomicExpandPass, fixing the bug above in the process. I am wondering on
what is the best way to cut such a patch in pieces for review. I have
attached the corresponding patches; they are not completely ready for
review (mostly missing tests), I would just like to make sure that the
general approach seems reasonable.

If so, I will finish them and send them on Phabricator for review.

Thanks,
Robin



On Fri, Aug 15, 2014 at 2:26 AM, Tim Northover <t.p.northover at
gmail.com>
wrote:
> > From my reading of Atomics.rst, it would be sound to reorder (It does
not
> > say much about load-linked, so I am treating it as a normal load here)
> >
> >> store seq_cst
> >> fence release
> >> load-linked monotonic
> >
> > into
> >
> >> load-linked monotonic
> >> store seq_cst
> >> fence release
>
> > Which would make an execution ending in %old_x = %old_y = 0 possible,
> while
> > it is impossible in the original program.
>
> Hmm, evil. Well, I'm convinced. Thanks very much for taking the time
> to come up with an example and telling us about it.
>
> > Fixing it is a two line change in insertLeadingFence, but it triggers
> some
> > test failures, both because of tests looking specifically for a fence
> > release here,
>
> That's fine, we can change those easily enough. And the "dmb
ishst"
> (as I understand it, it *is* a release fence, but not almost certainly
> not suitable for preventing this reordering).
>
> Cheers.
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-hooks-to-TargetLowering-for-a-future-AtomicExpan.patch
Type: text/x-patch
Size: 5511 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Copy-AtomicExpandLoadLinkedPass-X86AtomicExpandPass-.patch
Type: text/x-patch
Size: 21489 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-X86-Use-AtomicExpandPass-instead-of-X86AtomicExpandP.patch
Type: text/x-patch
Size: 5931 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Erase-X86AtomicExpandPass.patch
Type: text/x-patch
Size: 12605 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-ARM-Use-AtomicExpandPass-instead-of-AtomicExpandLoad.patch
Type: text/x-patch
Size: 66115 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-AArch64-Use-AtomicExpandPass-instead-of-AtomicExpand.patch
Type: text/x-patch
Size: 4807 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Erase-AtomicExpandLoadLinkedPass.patch
Type: text/x-patch
Size: 20517 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/ab02f8a5/attachment-0006.bin>

Hi Robin,

On 15 August 2014 21:49, Robin Morisset <morisset at google.com>
wrote:
> I have
> attached the corresponding patches; they are not completely ready for
review
> (mostly missing tests), I would just like to make sure that the general
> approach seems reasonable.
I've had a quick glance at the patches, and the code seems fairly sane.

But I'm not so sure about starting with a new pass then deleting the
other two. I think it's likely to muddy the revision control history.
I'd prefer to see a gradual evolution of the
AtomicExpandLoadLinked.cpp (perhaps starting with a "git mv" and some
internal renaming to stake out the intent, followed by adding and
using the extra hooks).

Cheers.

Tim.