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

[Sebastian Pop via llvm-commits]

What about disabling GVN-hoist and keep it on for -Os and -Oz which
are less used than -O2 and -O3?

On Thu, Jul 21, 2016 at 4:43 PM, Mehdi Amini <mehdi.amini at apple.com> 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.
>
>
> —
> Mehdi
>