> 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: <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: <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: <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: <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: <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: <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: <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: <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.