<div dir="ltr">this would be a bad idea until the scaling issues are resolved :)<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 21, 2016 at 6:07 PM, Sebastian Pop <span dir="ltr"><<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">What about disabling GVN-hoist and keep it on for -Os and -Oz which<br>
are less used than -O2 and -O3?<br>
<div class="HOEnZb"><div class="h5"><br>
On Thu, Jul 21, 2016 at 4:43 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
><br>
> On Jul 21, 2016, at 2:16 PM, David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>><br>
> wrote:<br>
><br>
><br>
><br>
> On Thu, Jul 21, 2016 at 1:55 PM, Mehdi AMINI via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> mehdi_amini added a comment.<br>
>><br>
>> In <a href="https://reviews.llvm.org/D22639#491753" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22639#491753</a>, @bkramer wrote:<br>
>><br>
>> > The problem is not a Halide test but a scalability issue in MemorySSA<br>
>> > that's making Halide unusable, consuming gigabytes of memory and taking<br>
>> > forever. Personally, I don't think the GVNHoist pass is ready for prime time<br>
>> > yet and needs more testing, like we did for other passes that were first<br>
>> > rigorously tested under a flag before being enabled by default. Going<br>
>> > directly from zero to default with a major new optimization infrastructure<br>
>> > (MemSSA) is a recipe for disaster.<br>
>><br>
>><br>
>> I agree with all of that and I missed the fact that it was not the regular<br>
>> plain old GVN that we were talking here but a new (and apparently still<br>
>> "experimental") pass.<br>
>> With this in mind, a cl::opt flag seem appropriate and I waive my previous<br>
>> concern.<br>
>> Thanks for the extra information!<br>
><br>
><br>
> Having fixed a number of bugs in GVNHoist already, I think that the pass<br>
> should be disabled for the 3.9 release.<br>
><br>
> I think that it would be prudent for new passes, like GVNHoist, to get<br>
> disabled if there are open miscompiles against them for the first few<br>
> months.<br>
><br>
> This way we let the new pass get exposed to code while also preserving the<br>
> correctness of our output when we know the pass is broken.<br>
><br>
> Otherwise I fear we will never be confident in turning the pass on by<br>
> default.<br>
><br>
> With all that said, I think that gvn-hoist should be disabled in trunk until<br>
> pr28606 and Alina's example are resolved.<br>
><br>
><br>
> +1 for disabling by default in 3.9 (with a cl::opt to enable it), and<br>
> switching the default to enabled in master as soon as (enough of) known bugs<br>
> are solved.<br>
><br>
><br>
> —<br>
> Mehdi<br>
><br>
</div></div></blockquote></div><br></div>