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

[Mehdi Amini via llvm-commits]

My understanding is that:

- O2 is supposed to be “best runtime performance, but don’t blow compile-time though"
- O3 is supposed to be: “allow some extra compile-time on top of O2"
- Os is supposed to be “tradeoff ‘some' runtime performance for code size”
- Oz is supposed to be “ ’never’ tradeoff code size for extra performance”

If we agree on this, how does your proposal fits in this?


> On Jul 21, 2016, at 6:07 PM, Sebastian Pop <sebpop at gmail.com> wrote:
> 
> 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
>>