[PATCH] D22639: Add flag to PassManagerBuilder to disable GVN Hoist Pass.

[Sean Silva via llvm-commits]

On Thu, Jul 21, 2016 at 2:43 PM, Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> On Jul 21, 2016, at 2:16 PM, David Majnemer <david.majnemer at gmail.com>
> wrote:
>
>
>
> On Thu, Jul 21, 2016 at 1:55 PM, Mehdi AMINI via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> mehdi_amini added a comment.
>>
>> In https://reviews.llvm.org/D22639#491753, @bkramer wrote:
>>
>> > 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.
>>
>>
>> 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.
>> With this in mind, a cl::opt flag seem appropriate and I waive my
>> previous concern.
>> Thanks for the extra information!
>>
>
> Having fixed a number of bugs in GVNHoist already, I think that the pass
> should be disabled for the 3.9 release.
>
> 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.
>
> 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.
>
> Otherwise I fear we will never be confident in turning the pass on by
> default.
>
> With all that said, I think that gvn-hoist should be disabled in trunk
> until pr28606 and Alina's example are resolved.
>
>
> +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.
>

+1

-- Sean Silva


>
>
> —
> Mehdi
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160721/bc9447b6/attachment.html>