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.
