On Thu, Nov 02, 2017 at 01:21:17PM +0100, Richard Biener wrote:
> On Wed, 1 Nov 2017, Marek Polacek wrote:
>
> > On Fri, Oct 27, 2017 at 12:46:12PM +0200, Richard Biener wrote:
> > > On Fri, 27 Oct 2017, Jakub Jelinek wrote:
> > >
> > > > On Fri, Oct 27, 2017 at 12:31:46PM +0200, Richard Biener wrote:
> > > > > I fear it doesn't work at all with LTO (you'll always get the old ABI
> > > > > if I read the patch correctly). This is because the function
> > > > > computing the size looks at flag_abi_version which isn't saved
> > > > > per function / TU.
> > > > >
> > > > > Similarly you'll never get the ABI warning with LTO (less of a big
> > > > > deal of course) because the langhook doesn't reflect things correctly
> > > > > either.
> > > > >
> > > > > So... can we instead compute whether a type is "empty" according
> > > > > to the ABI early and store the result in the type (thinking of
> > > > > doing this in layout_type?). Similarly set a flag whether to
> > > > > warn. Why do you warn from backends / code emission and not
> > > > > from the FEs? Is that to avoid warnings for calls that got inlined?
> > > > > Maybe the FE could set a flag on the call itself (ok, somewhat
> > > > > awkward to funnel through gimple).
> > > >
> > > > Warning in the FE is too early both because of the inlining, never
> > > > emitted functions and because whether an empty struct is passed
> > > > differently
> > > > from the past matters on the backend (whether its psABI says it should
> > > > be
> > > > not passed at all or not).
> > > >
> > > > Perhaps if empty types are rare enough it could be an artificial
> > > > attribute
> > > > on the type if we can't get a spare bit for that. But computing in the
> > > > FE
> > > > or before free_lang_data and saving on the type whether it is empty or
> > > > not
> > > > seems reasonable to me.
> > >
> > > There are 18 unused bits in tree_type_common if we don't want to re-use
> > > any. For the warning I first thought of setting TREE_NO_WARNING on it
> > > but that bit is used already. OTOH given the "fit" of TREE_NO_WARNING
> > > I'd move TYPE_ARTIFICIAL somewhere else.
> >
> > All right, should be done in the below. I've introduced two new flags,
> > TYPE_EMPTY_P (says whether the type is empty according to the psABI), and
> > TYPE_WARN_EMPTY_P (whether we should warn). I've added two new fields to
> > type_type_common and moved TYPE_ARTIFICIAL there; TYPE_WARN_EMPTY_P is now
> > mapped to nowarning_flag. So this should work with LTO, as demonstrated
> > by g++.dg/lto/pr60336_0.C.
> >
> > Regarding LTO and -Wabi warning, I've added Optimization to c.opt so that
> > we get warnings with LTO. But as pointed out IRC, this doesn't fully work
> > with cross-inlining. I tried to do some flags merging in inline_call, but
> > that didn't help, one of the problems is that warn_abi_version lives in
> > c-family only. Not sure if I'll be able to improve things here though.
> >
> > Bootstrapped/regtested on x86_64-linux, ppc64-linux, and aarch64-linux.
> > Bootstrap-lto passed on x86_64-linux and ppc64-linux.
>
> To me the tree.c stuff is_empty_type looks awfully ABI dependent
> and should thus reside in i386.c near the target hook implementation?
>
> What goes wrong if we do not introduce new int_maybe_empty_type_size
> and maybe_empty_type_size but instead change int_size_in_bytes and
> size_in_bytes to return 0 if TYPE_EMPTY_P ()? If the ABI can omit
> passing things assuming the size is zero should work as well, no?
> Otherwise I'd really prefer seeing explicit TYPE_EMPTY_P checks
> which would reduce the number of "indirect" greps one has to do when
> looking for effects of TYPE_EMPTY_P.
More on this in another mail.
> Otherwise the middle-end/LTO parts look ok.
Thanks.
> I'd omit the 'Optimization' change on the Wabi warning flag if it
> doesn't fully give us what we want and address this as a followup.
>
> I think 'Optimization' is also used for -help reporting and thus
> could be confusing at first.
Done. Alternatively we could change lto_handle_option to handle
even OPT_Wabi_ case. The current code looks dubious anyway: in
the OPT_Wabi case we change warn_psabi instead of warn_abi...
Marek
