Re: [PATCH] Deprecate -frepo option.

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jul 8, 2019 at 2:04 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 6/21/19 4:28 PM, Richard Biener wrote:
> > On Fri, Jun 21, 2019 at 4:13 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >>
> >> On Fri, Jun 21, 2019 at 04:04:00PM +0200, Martin Liška wrote:
> >>> On 6/21/19 1:58 PM, Jakub Jelinek wrote:
> >>>> On Fri, Jun 21, 2019 at 01:52:09PM +0200, Martin Liška wrote:
> >>>>> On 6/21/19 1:47 PM, Jonathan Wakely wrote:
> >>>>>> On Fri, 21 Jun 2019 at 11:40, Martin Liška wrote:
> >>>>>>> Yes, I would be fine to deprecate that for GCC 10.1
> >>>>>>
> >>>>>> Would it be appropriate to issue a warning in GCC 10.x if the option is used?
> >>>>>
> >>>>> Sure. With the patch attached one will see:
> >>>>>
> >>>>> $ gcc -frepo /tmp/main.cc -c
> >>>>> gcc: warning: switch ‘-frepo’ is no longer supported
> >>>>>
> >>>>> I'm sending patch that also removes -frepo tests from test-suite.
> >>>>> I've been testing the patch.
> >>>>
> >>>> IMHO for just deprecation of an option you don't want to remove it from the
> >>>> testsuite, just match the warning it will generate in those tests, and
> >>>> I'm not convinced you want to remove it from the documentation (rather than
> >>>> just saying in the documentation that the option is deprecated and might be
> >>>> removed in a later GCC version).
> >>>
> >>> Agree with you. I'm sending updated version of the patch.
> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> I'm also not convinced about the Deprecated flag, seems like that is a flag
> >> that we use for options that have been already removed.
> >> So, instead there should be some proper warning in the C++ FE for it,
> >> or just Warn.
> >
> > In principle -frepo is a nice idea - does it live up to its promises?  That is,
> > does it actually work, for example when throwing it on the libstdc++
> > testsuite or a larger C++ project?
>
> I've just tested tramp3d, and it does not survive linking:
>
> g++ tramp3d-v4.o
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> collect: recompiling tramp3d-v4.cpp
> collect: relinking
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: /tmp/ccEeuyj7.ltrans0.ltrans.o: in function `RefCountedBlockPtr<FieldEngineBaseData<3, Vector<3, double, Full>, ViewEngine<3, IndexFunction<GenericURM<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >::PositionsFunctor> > >, false, RefBlockController<FieldEngineBaseData<3, Vector<3, double, Full>, ViewEngine<3, IndexFunction<GenericURM<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >::PositionsFunctor> > > > >::RefCountedBlockPtr(RefCountedBlockPtr<FieldEngineBaseData<3, Vector<3, double, Full>, ViewEngine<3, IndexFunction<GenericURM<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >::PositionsFunctor> > >, false, RefBlockController<FieldEngineBaseData<3, Vector<3, double, Full>, ViewEngine<3, IndexFunction<GenericURM<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >::PositionsFunctor> > > > > const&)':
> <artificial>:(.text+0x4181b): undefined reference to `RefCountedPtr<RefBlockController<FieldEngineBaseData<3, Vector<3, double, Full>, ViewEngine<3, IndexFunction<GenericURM<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >::PositionsFunctor> > > > >::RefCountedPtr(RefCountedPtr<RefBlockController<FieldEngineBaseData<3, Vector<3, double, Full>, ViewEngine<3, IndexFunction<GenericURM<MeshTraits<3, double, UniformRectilinearTag, CartesianTag, 3> >::PositionsFunctor> > > > > const&)'
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: /tmp/ccEeuyj7.ltrans0.ltrans.o: in function `std::_Vector_base<Node<Range<3>, Interval<3> >, std::allocator<Node<Range<3>, Interval<3> > > >::_Vector_impl::~_Vector_impl()':
> <artificial>:(.text+0xc1890): undefined reference to `std::allocator<Node<Range<3>, Interval<3> > >::~allocator()'
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: /tmp/ccEeuyj7.ltrans0.ltrans.o: in function `std::_Vector_base<Node<Range<3>, Interval<3> >, std::allocator<Node<Range<3>, Interval<3> > > >::_Vector_base()':
> <artificial>:(.text+0xc18aa): undefined reference to `std::_Vector_base<Node<Range<3>, Interval<3> >, std::allocator<Node<Range<3>, Interval<3> > > >::_Vector_impl::_Vector_impl()'
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: /tmp/ccEeuyj7.ltrans0.ltrans.o: in function `std::vector<INode<3>, std::allocator<INode<3> > >::_S_use_relocate()':
> <artificial>:(.text+0xc496f): undefined reference to `std::vector<INode<3>, std::allocator<INode<3> > >::_S_nothrow_relocate(std::integral_constant<bool, true>)'
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: /tmp/ccEeuyj7.ltrans0.ltrans.o: in function `std::_Vector_base<Node<Range<3>, Interval<3> >, std::allocator<Node<Range<3>, Interval<3> > > >::~_Vector_base()':
> <artificial>:(.text+0xc4cc1): undefined reference to `std::_Vector_base<Node<Range<3>, Interval<3> >, std::allocator<Node<Range<3>, Interval<3> > > >::_M_deallocate(Node<Range<3>, Interval<3> >*, unsigned long)'
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: /tmp/ccEeuyj7.ltrans0.ltrans.o: in function `DataBlockPtr<double, false>::DataBlockPtr()':
> <artificial>:(.text+0xc6f96): undefined reference to `RefCountedBlockPtr<double, false, DataBlockController<double> >::RefCountedBlockPtr()'
> [... many other undefined references ...]
>
> Same happens also for GCC7. It does 17 iteration (#define MAX_ITERATIONS 17) and
> apparently 17 is not enough to resolve all symbols. And it's really slow.

Ouch.

> That said, I would recommend to remove it :)

In the end it's up to the C++ FE maintainers but the above clearly
doesn't look promising
(not sure if it keeps re-compiling _all_ repo-triggered templates or
just incrementally adds
them to new object files).

Also the docs suggest that -frepo is the only way to get template
instantiation commoning
if the target object format doesn't support sth like linkonce or
comdats.  Not sure if we need
to care though.  I don't remember any bugs about -frepo in many last
years but that could
be because it's perfect (well, it is not as seen above, but...).

I'm not opposed to removing -frepo from GCC 10 but then I would start
noting it is obsolete
on the GCC 9 branch at least.

Jason/Nathan?

Thanks,
Richard.

> Martin
>
> > The option doesn't document
> > optimization issues but I assume template bodies are not available
> > for IPA optimizations unless -frepo is combined with LTO where the
> > template CU[s] then bring them in.
> >
> > So I'm not sure - do we really want to remove this feature?
> >
> > Richard.
> >
> >>         Jakub
>