diff mbox

Add a combined_fn enum

Message ID 87fv0irod3.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Nov. 7, 2015, 12:22 p.m. UTC
I'm working on a patch series that needs to be able to treat built-in
functions and internal functions in a similar way.  This patch adds a
new enum, combined_fn, that combines the two together.  It also adds
utility functions for seeing which combined_fn (if any) is called by
a given CALL_EXPR or gcall.

Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
OK to install?

Thanks,
Richard


gcc/
	* tree-core.h (internal_fn): Move immediately after the definition
	of built_in_function.
	(combined_fn): New enum.
	* tree.h (as_combined_fn, builtin_fn_p, as_builtin_fn)
	(internal_fn_p, as_internal_fn): New functions.
	(get_call_combined_fn, combined_fn_name): Declare.
	* tree.c (get_call_combined_fn): New function.
	(combined_fn_name): Likewise.
	* gimple.h (gimple_call_combined_fn): Declare.
	* gimple.c (gimple_call_combined_fn): New function.

Comments

Jeff Law Nov. 9, 2015, 4:46 a.m. UTC | #1
On 11/07/2015 05:22 AM, Richard Sandiford wrote:
> I'm working on a patch series that needs to be able to treat built-in
> functions and internal functions in a similar way.  This patch adds a
> new enum, combined_fn, that combines the two together.  It also adds
> utility functions for seeing which combined_fn (if any) is called by
> a given CALL_EXPR or gcall.
>
> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
> OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> 	* tree-core.h (internal_fn): Move immediately after the definition
> 	of built_in_function.
> 	(combined_fn): New enum.
> 	* tree.h (as_combined_fn, builtin_fn_p, as_builtin_fn)
> 	(internal_fn_p, as_internal_fn): New functions.
> 	(get_call_combined_fn, combined_fn_name): Declare.
> 	* tree.c (get_call_combined_fn): New function.
> 	(combined_fn_name): Likewise.
> 	* gimple.h (gimple_call_combined_fn): Declare.
> 	* gimple.c (gimple_call_combined_fn): New function.
OK.
jeff
Bernd Schmidt Nov. 9, 2015, 10:13 a.m. UTC | #2
On 11/07/2015 01:22 PM, Richard Sandiford wrote:
> I'm working on a patch series that needs to be able to treat built-in
> functions and internal functions in a similar way.  This patch adds a
> new enum, combined_fn, that combines the two together.  It also adds
> utility functions for seeing which combined_fn (if any) is called by
> a given CALL_EXPR or gcall.
>
> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
> OK to install?

I see it's already acked, but have you considered just doing away with 
the builtin/internal function distinction?


Bernd
Richard Sandiford Nov. 9, 2015, 10:24 a.m. UTC | #3
Bernd Schmidt <bschmidt@redhat.com> writes:
> On 11/07/2015 01:22 PM, Richard Sandiford wrote:
>> I'm working on a patch series that needs to be able to treat built-in
>> functions and internal functions in a similar way.  This patch adds a
>> new enum, combined_fn, that combines the two together.  It also adds
>> utility functions for seeing which combined_fn (if any) is called by
>> a given CALL_EXPR or gcall.
>>
>> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
>> OK to install?
>
> I see it's already acked, but have you considered just doing away with 
> the builtin/internal function distinction?

I think they're too different to be done away with entirely.  built-in
functions map directly to a specific C-level callable function and
must have an fndecl, whereas no internal function should have an fndecl.
Whether a built-in function is available depends on the selected
language and what declarations the front-end has seen, while whether
an internal function is available depends entirely on GCC internal
information.  We always have the option of not handling a built-in
function specially and simply calling its function in the normal way,
whereas for internal functions we always need to opencode the function
in GCC.

Thanks,
Richard
Bernd Schmidt Nov. 9, 2015, 10:51 a.m. UTC | #4
On 11/09/2015 11:24 AM, Richard Sandiford wrote:
> Bernd Schmidt <bschmidt@redhat.com> writes:
>> I see it's already acked, but have you considered just doing away with
>> the builtin/internal function distinction?
>
> I think they're too different to be done away with entirely.  built-in
> functions map directly to a specific C-level callable function and
> must have an fndecl, whereas no internal function should have an fndecl.
> Whether a built-in function is available depends on the selected
> language and what declarations the front-end has seen, while whether
> an internal function is available depends entirely on GCC internal
> information.

Yes... but aren't these things fixable relatively easily (compared with 
what your patches are doing)?

I also have the problem that I can't quite see where your patch series 
is going. Let's take "Add internal bitcount functions", it adds new 
internal functions but no users AFAICS. What is the end goal here (there 
doesn't seem to be a [0/N] description in my inbox)?


Bernd
Richard Sandiford Nov. 9, 2015, 2:57 p.m. UTC | #5
Bernd Schmidt <bschmidt@redhat.com> writes:
> On 11/09/2015 11:24 AM, Richard Sandiford wrote:
>> Bernd Schmidt <bschmidt@redhat.com> writes:
>>> I see it's already acked, but have you considered just doing away with
>>> the builtin/internal function distinction?
>>
>> I think they're too different to be done away with entirely.  built-in
>> functions map directly to a specific C-level callable function and
>> must have an fndecl, whereas no internal function should have an fndecl.
>> Whether a built-in function is available depends on the selected
>> language and what declarations the front-end has seen, while whether
>> an internal function is available depends entirely on GCC internal
>> information.
>
> Yes... but aren't these things fixable relatively easily (compared with 
> what your patches are doing)?

I'm not sure what you mean by "fix" though.  I don't think we can change
any of the constraints above.

> I also have the problem that I can't quite see where your patch series 
> is going. Let's take "Add internal bitcount functions", it adds new 
> internal functions but no users AFAICS. What is the end goal here (there 
> doesn't seem to be a [0/N] description in my inbox)?

The main goal is to allow functions to be vectorised simply by defining
the associated optab.  At the moment you can get a scalar square root
instruction simply by defining something like sqrtdf2.  But if you want
to have vectorised sqrt, you need to have a target-specific C-level
built-in function for the vector form of sqrt, implement
TARGET_BUILTIN_VECTORIZED_FUNCTION, and expand the sqrt in the same
way that the target expands other directly-called built-in functions.
That seems unnecessarily indirect, especially when in practice those
target-specific functions tend to use patterns like sqrtv2df2 anyway.
It seems better to have GCC use the vector optabs directly.

This was prompted by the patch Dave Sherwood posted to support scalar
and vector fmin() and fmax() even with -fno-fast-math on aarch64.
As things stood we needed the same approach: use an optab for the scalar
version and TARGET_BUILTIN_VECTORIZED_FUNCTION for the vector version.
The problem is that at present there's no aarch64 built-in function that
does what we want, so we'd have to define a new one.  And that seems
silly when GCC really ought to be able to use the vector version
of the optab without any more help from the target.

I'm hoping to post those patches later today.

But the series has other side-benefits, like:

- allowing genmatch to fold calls to internal functions

- splitting the computational part of maths function from the
  the fallback errno handling at an earlier point, so that they
  get more optimisation

- clearly separating out the call folds that we're prepared to do
  on gimple, rather than calling into builtins.c.

Thanks,
Richard
Bernd Schmidt Nov. 9, 2015, 3:28 p.m. UTC | #6
On 11/09/2015 03:57 PM, Richard Sandiford wrote:

> I'm not sure what you mean by "fix" though.  I don't think we can change
> any of the constraints above.

I don't see why they couldn't be relaxed. What's stopping us from 
defining some builtins independently of the frontends, and don't all 
languages except Fortran use the normal builtins.def anyway? Also - if 
you can have an internal function without a fndecl, why wouldn't it work 
to allow that for builtins as well? But this is probably moot since I 
can now see a little more clearly what you want to do.

> The main goal is to allow functions to be vectorised simply by defining
> the associated optab.

Ok. So these internal functions would support arbitrary modes like 
V[1248][SDX]F etc., unlike the builtins? That wasn't entirely clear from 
the patches I looked at, it seemed like you were just duplicating every 
builtin as an internal function (something to which I would have objected).


Bernd
Richard Sandiford Nov. 9, 2015, 3:53 p.m. UTC | #7
Bernd Schmidt <bschmidt@redhat.com> writes:
> On 11/09/2015 03:57 PM, Richard Sandiford wrote:
>
>> I'm not sure what you mean by "fix" though.  I don't think we can change
>> any of the constraints above.
>
> I don't see why they couldn't be relaxed. What's stopping us from 
> defining some builtins independently of the frontends, and don't all 
> languages except Fortran use the normal builtins.def anyway?

I think the problem isn't so much with the functions that we define
ourselves as those that simply mirror a standard library function.
We can't assume that all the libm functions exist.  It's also valid
for the implementation to add target-specific attributes on top of
what the standard says (e.g. "nomips16" on MIPS).

So if a call starts out as a call to sinf and we treat it as a call to
__builtin_sinf for optimisation purposes, there is always conceptually
an underlying "physical" sinf function.  We also can't convert calls
to __builtin_sin to calls to __builtin_sinf without proof that a sinf
definition exists (not guaranteed for C90).

> Also - if you can have an internal function without a fndecl, why
> wouldn't it work to allow that for builtins as well? But this is
> probably moot since I can now see a little more clearly what you want
> to do.

I think that's the defining characteristic of builtins vs. internal
functions though.  built-in functions are directly callable by source
code and have a defined function signature.  Internal functions are not
directly callable by source code and have a flexible function signature.

There are cases where this difference doesn't matter, which is why
the combined_fn enum seemed like a good idea.  But I don't think we
can do away with the distinction entirely.

>> The main goal is to allow functions to be vectorised simply by defining
>> the associated optab.
>
> Ok. So these internal functions would support arbitrary modes like 
> V[1248][SDX]F etc., unlike the builtins?

Yeah.  They also never set errno.

> That wasn't entirely clear from the patches I looked at, it seemed
> like you were just duplicating every builtin as an internal function
> (something to which I would have objected).

Yeah, sorry about that.  I'll post the patches shortly.

Thanks,
Richard
diff mbox

Patch

diff --git a/gcc/gimple.c b/gcc/gimple.c
index 4ce38da..de3520a 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2530,6 +2530,27 @@  gimple_call_builtin_p (const gimple *stmt, enum built_in_function code)
   return false;
 }
 
+/* If CALL is a call to a combined_fn (i.e. an internal function or
+   a normal built-in function), return its code, otherwise return
+   CFN_LAST.  */
+
+combined_fn
+gimple_call_combined_fn (const gimple *stmt)
+{
+  if (const gcall *call = dyn_cast <const gcall *> (stmt))
+    {
+      if (gimple_call_internal_p (call))
+	return as_combined_fn (gimple_call_internal_fn (call));
+
+      tree fndecl = gimple_call_fndecl (stmt);
+      if (fndecl
+	  && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
+	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
+    }
+  return CFN_LAST;
+}
+
 /* Return true if STMT clobbers memory.  STMT is required to be a
    GIMPLE_ASM.  */
 
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 781801b..13cfbce 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1499,6 +1499,7 @@  extern tree gimple_signed_type (tree);
 extern alias_set_type gimple_get_alias_set (tree);
 extern bool gimple_ior_addresses_taken (bitmap, gimple *);
 extern bool gimple_builtin_call_types_compatible_p (const gimple *, tree);
+extern combined_fn gimple_call_combined_fn (const gimple *);
 extern bool gimple_call_builtin_p (const gimple *);
 extern bool gimple_call_builtin_p (const gimple *, enum built_in_class);
 extern bool gimple_call_builtin_p (const gimple *, enum built_in_function);
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index 954368f..afb53be 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -184,6 +184,35 @@  enum built_in_function {
   END_BUILTINS
 };
 
+/* Internal functions.  */
+enum internal_fn {
+#define DEF_INTERNAL_FN(CODE, FLAGS, FNSPEC) IFN_##CODE,
+#include "internal-fn.def"
+  IFN_LAST
+};
+
+/* An enum that combines target-independent built-in functions with
+   internal functions, so that they can be treated in a similar way.
+   The numbers for built-in functions are the same as for the
+   built_in_function enum.  The numbers for internal functions
+   start at END_BUITLINS.  */
+enum combined_fn {
+#define DEF_BUILTIN(ENUM, N, C, T, LT, B, F, NA, AT, IM, COND) \
+  CFN_##ENUM = int (ENUM),
+#include "builtins.def"
+
+#define DEF_BUILTIN(ENUM, N, C, T, LT, B, F, NA, AT, IM, COND)
+#define DEF_BUILTIN_CHKP(ENUM, N, C, T, LT, B, F, NA, AT, IM, COND) \
+  CFN_##ENUM##_CHKP = int (ENUM##_CHKP),
+#include "builtins.def"
+
+#define DEF_INTERNAL_FN(CODE, FLAGS, FNSPEC) \
+  CFN_##CODE = int (END_BUILTINS) + int (IFN_##CODE),
+#include "internal-fn.def"
+
+  CFN_LAST
+};
+
 /* Tree code classes.  Each tree_code has an associated code class
    represented by a TREE_CODE_CLASS.  */
 enum tree_code_class {
@@ -766,13 +795,6 @@  enum annot_expr_kind {
   annot_expr_kind_last
 };
 
-/* Internal functions.  */
-enum internal_fn {
-#define DEF_INTERNAL_FN(CODE, FLAGS, FNSPEC) IFN_##CODE,
-#include "internal-fn.def"
-  IFN_LAST
-};
-
 /*---------------------------------------------------------------------------
                                 Type definitions
 ---------------------------------------------------------------------------*/
diff --git a/gcc/tree.c b/gcc/tree.c
index 5b9a7bd..94c3a1a 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9316,6 +9316,25 @@  get_callee_fndecl (const_tree call)
   return NULL_TREE;
 }
 
+/* If CALL_EXPR CALL calls a normal built-in function or an internal function,
+   return the associated function code, otherwise return CFN_LAST.  */
+
+combined_fn
+get_call_combined_fn (const_tree call)
+{
+  /* It's invalid to call this function with anything but a CALL_EXPR.  */
+  gcc_assert (TREE_CODE (call) == CALL_EXPR);
+
+  if (!CALL_EXPR_FN (call))
+    return as_combined_fn (CALL_EXPR_IFN (call));
+
+  tree fndecl = get_callee_fndecl (call);
+  if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
+
+  return CFN_LAST;
+}
+
 #define TREE_MEM_USAGE_SPACES 40
 
 /* Print debugging information about tree nodes generated during the compile,
@@ -13805,5 +13824,18 @@  nonnull_arg_p (const_tree arg)
   return false;
 }
 
+/* Return the name of combined function FN, for debugging purposes.  */
+
+const char *
+combined_fn_name (combined_fn fn)
+{
+  if (builtin_fn_p (fn))
+    {
+      tree fndecl = builtin_decl_explicit (as_builtin_fn (fn));
+      return IDENTIFIER_POINTER (DECL_NAME (fndecl));
+    }
+  else
+    return internal_fn_name (as_internal_fn (fn));
+}
 
 #include "gt-tree.h"
diff --git a/gcc/tree.h b/gcc/tree.h
index 6768b3b..14b46a4 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -22,6 +22,58 @@  along with GCC; see the file COPYING3.  If not see
 
 #include "tree-core.h"
 
+/* Convert a target-independent built-in function code to a combined_fn.  */
+
+inline combined_fn
+as_combined_fn (built_in_function fn)
+{
+  return combined_fn (int (fn));
+}
+
+/* Convert an internal function code to a combined_fn.  */
+
+inline combined_fn
+as_combined_fn (internal_fn fn)
+{
+  return combined_fn (int (fn) + int (END_BUILTINS));
+}
+
+/* Return true if CODE is a target-independent built-in function.  */
+
+inline bool
+builtin_fn_p (combined_fn code)
+{
+  return int (code) < int (END_BUILTINS);
+}
+
+/* Return the target-independent built-in function represented by CODE.
+   Only valid if builtin_fn_p (CODE).  */
+
+inline built_in_function
+as_builtin_fn (combined_fn code)
+{
+  gcc_checking_assert (builtin_fn_p (code));
+  return built_in_function (int (code));
+}
+
+/* Return true if CODE is an internal function.  */
+
+inline bool
+internal_fn_p (combined_fn code)
+{
+  return int (code) >= int (END_BUILTINS);
+}
+
+/* Return the internal function represented by CODE.  Only valid if
+   internal_fn_p (CODE).  */
+
+inline internal_fn
+as_internal_fn (combined_fn code)
+{
+  gcc_checking_assert (internal_fn_p (code));
+  return internal_fn (int (code) - int (END_BUILTINS));
+}
+
 /* Macros for initializing `tree_contains_struct'.  */
 #define MARK_TS_BASE(C)					\
   do {							\
@@ -4431,6 +4483,7 @@  extern unsigned crc32_unsigned (unsigned, unsigned);
 extern void clean_symbol_name (char *);
 extern tree get_file_function_name (const char *);
 extern tree get_callee_fndecl (const_tree);
+extern combined_fn get_call_combined_fn (const_tree);
 extern int type_num_arguments (const_tree);
 extern bool associative_tree_code (enum tree_code);
 extern bool commutative_tree_code (enum tree_code);
@@ -4456,6 +4509,7 @@  extern tree lhd_gcc_personality (void);
 extern void assign_assembler_name_if_neeeded (tree);
 extern void warn_deprecated_use (tree, tree);
 extern void cache_integer_cst (tree);
+extern const char *combined_fn_name (combined_fn);
 
 /* Return the memory model from a host integer.  */
 static inline enum memmodel