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

[Daniel Berlin via llvm-commits]

this would be a bad idea until the scaling issues are resolved :)


On Thu, 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160721/1003a90d/attachment.html>