Re: [PR 77333] Fix fntypes of calls calling clones

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Mon, Mar 13, 2017 at 01:46:47PM +0100, Richard Biener wrote:
> On Fri, 10 Mar 2017, Martin Jambor wrote:
> 
> > Hi,
> > 
> > PR 77333 is a i686-windows target bug, which however has its root in
> > our general mechanism of adjusting gimple statements when redirecting
> > call graph edge.  Basically, these three things trigger it:
> > 
> > 1) IPA-CP figures out that the this parameter of a C++ class method is
> >    unused and because the class is in an anonymous namespace, it can
> >    be removed and all calls adjusted.  That effectively changes a
> >    normal method into a static method and so internally, its type
> >    changes from METHOD_TYPE to FUNCTION_TYPE.
> > 
> > 2) Since the fix of PR 57330, we do not update gimple_call_fntype to
> >    match the new type, in fact we explicitely set it to the old, now
> >    invalid, type (see redirect_call_stmt_to_callee in cgraph.c).
> > 
> > 3) Function ix86_get_callcvt which decides on call ABI, ends with the
> >    following condition:
> > 
> >      if (ret != 0
> >          || is_stdarg
> >          || TREE_CODE (type) != METHOD_TYPE
> >          || ix86_function_type_abi (type) != MS_ABI)
> >        return IX86_CALLCVT_CDECL | ret;
> > 
> >      return IX86_CALLCVT_THISCALL;
> > 
> >    ...and since now the callee is no longer a METHOD_TYPE but callers
> >    still think that they are, leading to calling convention mismatches
> >    and subsequent crashes.  It took me quite a lot of time to come up
> >    with a small testcase (reproducible using wine) but eventually I
> >    managed.
> > 
> > The fix is not to do 2) above, but doing so without re-introducing PR
> > 57330, of course.  There are two options.  One is to use the
> > call_stmt_cannot_inline_p flag of call-graph edges and not do any
> > IPA-CP accross those edges.  That is done in the patch below.  The (so
> > far a bit hypothetical) problem with that approach is that the call
> > graph edge flag may not be 100% reliable in LTO, because incompatible
> > decls might get merged and then we wold re-introduce PR 57330 again -
> > only with on invalid code and with LTO but an ICE nevertheless.
> 
> So you mean replacing a matching decl with a non-matching and thus
> during symtab merge introduce the issue.  You could detect whenever
> merging incompatible symbols and set ->cannot_change_signature on
> the prevailing node though?
> 
> But how do we deal with devirt here?  That is propagation itself
> may introduce (knowledge of) the incompatibility...

True, I haven't thought about that.

> 
> In general I am sympathetic with not doing any IPA propagation
> across call stmt signature incompatibilties.  Of course we may
> be still too strict in those compatibility check...
> 
> > So the alternative would be to re-check when doing the gimple
> > statement adjustment and if the types match, then set the correct new
> > gimple_fntype and if they don't... then we can either leave it be or
> > just run the same type transformation on it as we did on the callee,
> > though they would be bogus either way.  That is implemented in the
> > attached patch.
> 
> As we _do_ adjust the call (apply the transform to its actual
> arguments) we should apply the same transform to
> gimple_call_fntype.  We can avoid doing duplicate work in case
> the old fndecl (do we still have that around?) matched fntype.
> 
> I think the common case of "mismatches" is having them with
> respect to the "extern" declaration but then the implementation
> is actually ok (otherwise people would run into runtime issues).
> 
> I wonder what we do to IPA-SRA-kind of transforms ... (as it's early
> it's much easier to just not do anything about those at this point).
> 
> > I have successfully bootstrapped both patches on x86_64-linux and I
> > have also tested them both on a windows cross-compiler and wine (with
> > some noise but I believe it is just noise).
> > 
> > Honza, Richi, do you prefer any one approach over the other?
> > Actually, we can have both, would that be desirable?
> 
> I think I'd like to see statistics for say, SPEC, how many
> cgraph edges we disable transforms for.

I used some old SPEC 2k6 config file, which meant that 400.perlbench,
416.gamess, 447.dealII and 450.soplex failed for some reason in normal
build and 400.perlbench, 416.gamess, 435.gromacs, 436.cactusADM,
447.dealII, 450.soplex, 454.calculix in the LTO-build.

In non-LTO build we did not seem to disable anything, in LTO one
however, surprisingly many:

  Number of function clonings performed by IPA-CP:
  | Build   | Unpatched | Patched |
  |---------+-----------+---------|
  | non-LTO |      1598 |    1598 |
  | LTO     |      5489 |    4881 |

> For correctness I'd simply
> do the same transform to fntype as to the actual stmt (which of course
> has some cost).
> 

After talking to Honza today, we decided to probably go this route and
use the patch doing the type conversion at acall-sites when necessary.
Honza promised to review the patch soon (he wants to figure out why
former_clone_of can be NULL, something I decided not to bother about
since at that time I thought the other approach was going to be
preferable).

Thanks,

Martin