On 11/28/18 6:35 AM, Richard Biener wrote:
On Tue, Nov 27, 2018 at 3:52 AM Martin Sebor <mse...@gmail.com> wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01759.html
If there are no objections or suggestions for tweaks I'll commit
this updated comment this week.
Please do not commit such changes w/o approval.
Let me follow up on this separately.
Does the new comments match existing usage, for example in gimplify.c
where we do
static void
build_stack_save_restore (gcall **save, gcall **restore)
{
tree tmp_var;
*save = gimple_build_call (builtin_decl_implicit (BUILT_IN_STACK_SAVE), 0);
tmp_var = create_tmp_var (ptr_type_node, "saved_stack");
gimple_call_set_lhs (*save, tmp_var);
*restore
= gimple_build_call (builtin_decl_implicit (BUILT_IN_STACK_RESTORE),
1, tmp_var);
}
?
I don't know why this code uses builtin_decl_implicit here. I don't
think it's necessary or even correct. If builtin_decl_implicit were
to return null gimple_build_call() would ICE. Replacing it with
builtin_decl_explicit passes an x86_64 bootstrap and regression test.
I see the same pattern in some of the other calls to
builtin_decl_implicit with non-library built-ins. E.g., in
gimplify_function_tree:
x = builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS);
call = gimple_build_call (x, 1, integer_zero_node);
+/* Return the tree node for the built-in function declaration corresponding
+ to FNCODE if its IMPLICIT_P flag has been set or NULL otherwise.
+ IMPLICIT_P is clear for library built-ins that GCC implements but that
+ may not be implemented in the runtime library on the target. See also
+ the DEF_BUILTIN macro in builtins.def. */
+
it isn't exactly helpful to specify when IMPLICIT_P is not set. When is it
actually set?
My understanding of the difference is that the compiler may create a call
to a builtin_decl_implicit decl but _not_ to a builtin_decl_explicit one
unless a call to such function already existed.
This sounds like an important detail to capture but I'm not sure
I quite understand it to do so.
Besides the BUILT_IN_STACK question above, cp/cp-gimplify.c for
example calls builtin_decl_explicit with BUILT_IN_UNREACHABLE
to unconditionally insert a call to the built-in. It does that
whether or not a call to the built-in has been seen in the program.
E.g., this:
int f () { }
int g () { return f (); }
ends up with a __builtin_unreachable call (IMPLICIT is set here
so the result would be the same if cp/cp-gimplify.c called
builtin_decl_implicit instead, but that seems beside the point).
Note we set the implicit flag during gimplification if we see such decl
referenced and it is marked as properly declared by the FE.
I see this in gimplify.c:
/* If we see a call to a declared builtin or see its address
being taken (we can unify those cases here) then we can mark
the builtin for implicit generation by GCC. */
if (TREE_CODE (op0) == FUNCTION_DECL
&& fndecl_built_in_p (op0, BUILT_IN_NORMAL)
&& builtin_decl_declared_p (DECL_FUNCTION_CODE (op0)))
set_builtin_decl_implicit_p (DECL_FUNCTION_CODE (op0), true);
This makes sense to me for library built-ins but for others like
__builtin_unreachable it seems unnecessary. Their IMPLICIT flag
is already set. The only calls to set_builtin_decl_implicit_p
that I can find that clear the IMPLICIT flag are in the VMS
back-end for BUILT_IN_FWRITE (apparently because the function
is non-standard there).
Overall your patch is an improvement I think but maybe you can
refer to builtins.def in the documentation for builtin_decl_implicit?
Sure, I'm happy to adjust it once we're confident our understanding
of how the flag works is correct. In light of your comments I for
one don't feel I'm there yet.
Thanks
Martin
On 11/20/18 1:24 PM, Martin Sebor wrote:
On 11/20/2018 11:02 AM, Martin Sebor wrote:
Would the updated comments in the attached patch more accurately
describe the purpose of the IMPLICIT_P flag and
the builtin_decl_explicit() and builtin_decl_implicit() functions?
I ended up here while trying to understand the differences between
the functions on different targets and decide which one should be
used to diagnose bugs like the one below:
long double fabsl (); // missing prototype
long double f (int x)
{
return fabsl (x); // want a warning here
}
I think we want the warning regardless of IMPLICIT_P so the warning
code should call builtin_decl_explicit() to obtain fabsl's expected
type, even if the target's runtime doesn't support the function on
the basis of the comment:
When a program uses floorf we may assume that the floorf function
has the expected meaning
Actually, some more testing suggests the comment in builtins.def
(either the original or the patched one) isn't entirely accurate
or helpful to understanding the purpose of the flag:
IMPLICIT specifies condition when the builtin can be produced by
compiler. For instance C90 reserves floorf function, but does not
define it's meaning. When user uses floorf we may assume that the
floorf has the meaning we expect, but we can't produce floorf by
simplifying floor((double)float) since the runtime need not
implement it.
Given the following code:
float floorf ();
void f (void)
{
if (floorf (0.0f))
__builtin_abort ();
}
in C90 mode, BUILT_IN_FLOORF's IMPLICIT flag is clear and GCC
doesn't seem to assume anything about the call to the function,
contrary to the comment ("we may assume the meaning we expect").
The comment also doesn't explain when IMPLICIT may be set.
I've updated the comment a bit more to more accurately describe
when I think the flag is set or clear, and how it's used.
Corrections or further clarification are appreciated.
Thanks
Martin
