>>On Fri, Nov 18, 2016 at 8:43 AM, Philip Reames via llvm-dev ><llvm-dev at lists.llvm.org> wrote: >> Glad to see this landing! It's been a long time coming. >> >> Once this is in, please do not turn it on by default immediately. Let's >> call for volunteers to find some of the most egregious miscompiles, fix >> them, and then turn this on by default. >> >There are no immediate plans to enable NewGVN by default (at least, >not in the near future). In fact, the mail that I originally wrote >doesn't at all mention the switch, neither any follow-ups from me or >Daniel, so, I'm not entirely sure where you got that idea from. If you >take a look more closely (at the mail, or a the patch), you'll realize >that "key" pieces that are in old GVN are still missing. The most >noticeable are PRE and load coercion. In other words, the patch >proposed is not (yet) on par with what the current GVN does (although >all the missing pieces are already implemented out-of-tree). >Also, let me try to clarify one point. This is already a call for >volunteers. If you feel adventurous, you can download the >patch/apply/test/report issues. I can and I will spend timei>ntegrating the rest of the work and fix all the reported>bugs/miscompiles. If there's something that can we do in a cleaner >way, a discussion will happen on the mailing list/on the review thread >and everybody will have a chance to comment, as it's happening for the >initial patch (and as I always try to do). >Once the first patch lands, I'll commit a temporary cl::opt to enable >NewGVN for those interested in testing and send another CFT e-mail. >FWIW, The patch had already a round of light testing internally. Of >course, this is not enough or indicative of its maturity/robustness. I >plan to have it tested more carefully inside my organization in >parallel. >That said, thanks for you input.Why even commit the patch to enable the pass into trunk. Just leave it up on Phabricator for testers to apply locally to their tree? Jack>-- >Davide
On Tue, Nov 22, 2016 at 10:27 AM, Jack Howarth via llvm-dev < llvm-dev at lists.llvm.org> wrote:> >>On Fri, Nov 18, 2016 at 8:43 AM, Philip Reames via llvm-dev > ><llvm-dev at lists.llvm.org> wrote: > >> Glad to see this landing! It's been a long time coming. > >> > >> Once this is in, please do not turn it on by default immediately. Let's > >> call for volunteers to find some of the most egregious miscompiles, fix > >> them, and then turn this on by default. > >> > >There are no immediate plans to enable NewGVN by default (at least, > >not in the near future). In fact, the mail that I originally wrote > >doesn't at all mention the switch, neither any follow-ups from me or > >Daniel, so, I'm not entirely sure where you got that idea from. If you > >take a look more closely (at the mail, or a the patch), you'll realize > >that "key" pieces that are in old GVN are still missing. The most > >noticeable are PRE and load coercion. In other words, the patch > >proposed is not (yet) on par with what the current GVN does (although > >all the missing pieces are already implemented out-of-tree). > >Also, let me try to clarify one point. This is already a call for > >volunteers. If you feel adventurous, you can download the > >patch/apply/test/report issues. I can and I will spend time > i>ntegrating the rest of the work and fix all the reported > >bugs/miscompiles. If there's something that can we do in a cleaner > >way, a discussion will happen on the mailing list/on the review thread > >and everybody will have a chance to comment, as it's happening for the > >initial patch (and as I always try to do). > >Once the first patch lands, I'll commit a temporary cl::opt to enable > >NewGVN for those interested in testing and send another CFT e-mail. > >FWIW, The patch had already a round of light testing internally. Of > >course, this is not enough or indicative of its maturity/robustness. I > >plan to have it tested more carefully inside my organization in > >parallel. > >That said, thanks for you input. > > Why even commit the patch to enable the pass into trunk. Just leave it > up on Phabricator for testers to apply locally to their tree? > Jack > >Because that increase friction to testing it. Asking people to pass -mllvm -newgvn will end up with a lot more testing than "please apply this patch and recompile everything". - Michael Spencer> > >-- > >Davide > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161125/fd582a64/attachment.html>
Right. On trunk I can have it run through swathes of internal test code with a new flag in a matter of mouse clicks through our CI. Having to apply a patch manually is a more involved affair with a lot more manual steps. That's not to say it can't be done, but it's something I'd rather avoid where a far easier solution exists. At that point it becomes a piece of work that I have to schedule and prioritise vs my other responsibilities (and therefore might get done much later or not at all). If it's just kicking off an automated test run and reporting any bugs that turn up it's probably something I can just squeeze in straight away without any real planning. -Greg On Fri, 25 Nov 2016 at 18:46, Michael Spencer via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Tue, Nov 22, 2016 at 10:27 AM, Jack Howarth via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > >>On Fri, Nov 18, 2016 at 8:43 AM, Philip Reames via llvm-dev > ><llvm-dev at lists.llvm.org> wrote: > >> Glad to see this landing! It's been a long time coming. > >> > >> Once this is in, please do not turn it on by default immediately. Let's > >> call for volunteers to find some of the most egregious miscompiles, fix > >> them, and then turn this on by default. > >> > >There are no immediate plans to enable NewGVN by default (at least, > >not in the near future). In fact, the mail that I originally wrote > >doesn't at all mention the switch, neither any follow-ups from me or > >Daniel, so, I'm not entirely sure where you got that idea from. If you > >take a look more closely (at the mail, or a the patch), you'll realize > >that "key" pieces that are in old GVN are still missing. The most > >noticeable are PRE and load coercion. In other words, the patch > >proposed is not (yet) on par with what the current GVN does (although > >all the missing pieces are already implemented out-of-tree). > >Also, let me try to clarify one point. This is already a call for > >volunteers. If you feel adventurous, you can download the > >patch/apply/test/report issues. I can and I will spend time > i>ntegrating the rest of the work and fix all the reported > >bugs/miscompiles. If there's something that can we do in a cleaner > >way, a discussion will happen on the mailing list/on the review thread > >and everybody will have a chance to comment, as it's happening for the > >initial patch (and as I always try to do). > >Once the first patch lands, I'll commit a temporary cl::opt to enable > >NewGVN for those interested in testing and send another CFT e-mail. > >FWIW, The patch had already a round of light testing internally. Of > >course, this is not enough or indicative of its maturity/robustness. I > >plan to have it tested more carefully inside my organization in > >parallel. > >That said, thanks for you input. > > Why even commit the patch to enable the pass into trunk. Just leave it > up on Phabricator for testers to apply locally to their tree? > Jack > > > Because that increase friction to testing it. Asking people to pass -mllvm > -newgvn will end up with a lot more testing than "please apply this patch > and recompile everything". > > - Michael Spencer > > > > >-- > >Davide > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161125/7a655614/attachment.html>