
<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=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 21, 2016, at 2:16 PM, David Majnemer <<a href="mailto:david.majnemer@gmail.com" class="">david.majnemer@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jul 21, 2016 at 1:55 PM, Mehdi AMINI via llvm-commits <span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">mehdi_amini added a comment.<br class="">

<span class=""><br class="">

In <a href="https://reviews.llvm.org/D22639#491753" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D22639#491753</a>, @bkramer wrote:<br class="">

<br class="">

> The problem is not a Halide test but a scalability issue in MemorySSA that's making Halide unusable, consuming gigabytes of memory and taking forever. Personally, I don't think the GVNHoist pass is ready for prime time yet and needs more testing, like we did for other passes that were first rigorously tested under a flag before being enabled by default. Going directly from zero to default with a major new optimization infrastructure (MemSSA) is a recipe for disaster.<br class="">

<br class="">

<br class="">

</span>I agree with all of that and I missed the fact that it was not the regular plain old GVN that we were talking here but a new (and apparently still "experimental") pass.<br class="">

With this in mind, a cl::opt flag seem appropriate and I waive my previous concern.<br class="">

Thanks for the extra information!<br class=""></blockquote><div class=""><br class=""></div><div class="">Having fixed a number of bugs in GVNHoist already, I think that the pass should be disabled for the 3.9 release.</div><div class=""><br class=""></div><div class="">I think that it would be prudent for new passes, like GVNHoist, to get disabled if there are open miscompiles against them for the first few months.</div><div class=""><br class=""></div><div class="">This way we let the new pass get exposed to code while also preserving the correctness of our output when we know the pass is broken.</div><div class=""><br class=""></div><div class="">Otherwise I fear we will never be confident in turning the pass on by default.</div><div class=""><br class=""></div><div class="">With all that said, I think that gvn-hoist should be disabled in trunk until pr28606 and Alina's example are resolved.</div></div></div></div></div></blockquote><div><br class=""></div><div>+1 for disabling by default in 3.9 (with a cl::opt to enable it), and switching the default to enabled in master as soon as (enough of) known bugs are solved.</div><div><br class=""></div><div><br class=""></div><div>— </div><div>Mehdi</div></div><br class=""></body></html>