Re: [PATCH] match.pd: Punt on optimizing sqrt with incorrect arg type [PR105150]

[Richard Biener <rguenther@suse.de>]

On Tue, 5 Apr 2022, Jakub Jelinek wrote:

> On Tue, Apr 05, 2022 at 10:50:01AM +0200, Richard Biener wrote:
> > > In the following testcase sqrt is declared as unprototyped function
> > > and so it depends on what type has the argument passed to it.
> > > If an argument of incorrect type is passed, the sqrt comparison
> > > simplification can create invalid GIMPLE.
> > > 
> > > The patch fixes it by punting if there is a mismatch of types.
> > > As I didn't want to reindent 133 lines, the first hunk contains an
> > > ugly hack with if (false).  If you prefer reindentation, I can do that
> > > too.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > So why does this get pas gimple_call_combined_fn ()?  Are those
> > internal functions at the point we match them?  Otherwise we
> > invoke gimple_builtin_call_types_compatible_p on them.  If
> > they are internal functions why did we rewrite the calls to them
> > when the types do not match?  If gimple_builtin_call_types_compatible_p
> > is fine then why did the frontend assign BUILT_IN_SQRT to the
> > function decls?
> 
> For the unprototyped functions like double sqrt (); they can be called in a
> valid way or they might not, so I think having DECL_BUILT_IN_CLASS other
> than BUILT_IN_NORMAL is right.
> 
> The problem is that we don't do proper checking.
> 
> In generic-match.cc, we just call get_call_combined_fn which performs no
> checking of the arguments whatsoever.
> 
> In gimple-match.cc, we call gimple_call_combined_fn, which performs bad
> checking of the arguments.
> 
> When not an internal function (which doesn't need argument checking, the
> compiler guarantees it is right), the former does just:
>   tree fndecl = get_callee_fndecl (call);
>   if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
>     return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
> In GIMPLE, we call:
>   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
> but that is insufficient, that verifies whether the arguments passed to
> GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
> be the user declaration, so doesn't have to match exactly the builtin
> as predeclared by builtins.def etc. (though, there is the cotcha that say
> for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
> we use additional:
>   tree callee = gimple_call_fndecl (stmt);
>   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
>   if (decl
>       && decl != callee
>       && !gimple_builtin_call_types_compatible_p (stmt, decl))
>     return false;

Yeah, I think we should use that (and only that) function decl
in get_call_combined_fn and gimple_call_combined_fn until the
frontend stops slapping wrong BUILT_IN_* on random decls.

> For this particular testcase, seems the problem is just the generic folding,
> if I add && GIMPLE to the sqrt comparison foldings, it doesn't ICE anymore,
> so just adding generic_builtin_call_types_compatible_p (basically copy
> gimple_builtin_call_types_compatible_p) might be enough.
> Though a big question is what it should do, using useless_type_conversion_p
> might be needed so that it can deal e.g. with the FILE * vs. void *
> differences (and various others), on the other side, what e.g. the exact
> sqrt comparison folding requires to be much more strict, as it turns
> (cmp (sq @0) REAL_CST@1)
> to
> (cmp @0 @1)
> and similarly for
> (cmp (sq @0) (sq @1))
> and that requires exact type match, no?

Yes, on GENERIC it requires types_match (), so get_call_combined_fn
should use a type checking variant that checks that instead of
useless_type_conversion_p.

> So one possibility would be to use useless_type_conversion_p but actually
> not write (cmp @0 @1) in such cases, but (cmp (convert:op1type @0) @1)
> or so?

No, I think the match.pd patterns are all fine and the fix has to be
elsewhere.  Either in the generic get_call_combined_fn or in
extra plumbing emitted by genmatch (I prefer to fix get_call_combined_fn
and friends since we already do some checking for GIMPLE).  Well,
of course I prefer to have the frontend fixed, but ...

Richard.