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.
Thanks for taking the time to look at the patches. I will redo them in the way you suggest and put them on Phabricator for review. Cheers, Robin On Mon, Aug 18, 2014 at 6:16 AM, Tim Northover <t.p.northover at gmail.com> wrote:> 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. >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140818/9fdb74f4/attachment.html>
First batch of patches for review: http://reviews.llvm.org/D4958 http://reviews.llvm.org/D4959 http://reviews.llvm.org/D4960 http://reviews.llvm.org/D4961 These fix the problem exposed above (and another I found since), and impact exclusively with the ARM backend. I have another patch coming soon offering more refactoring, and merging with X86AtomicExpandPass. Do you want to be added as subscribers/reviewers ? (I only put Tim Northover as subscriber on the first batch because he looked interested in that bug). Cheers, Robin On Mon, Aug 18, 2014 at 8:55 AM, Robin Morisset <morisset at google.com> wrote:> Thanks for taking the time to look at the patches. > > I will redo them in the way you suggest and put them on Phabricator for > review. > > Cheers, > Robin > > > On Mon, Aug 18, 2014 at 6:16 AM, Tim Northover <t.p.northover at gmail.com> > wrote: > >> 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. >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140818/720cd865/attachment.html>