diff mbox

Speed-up def_builtin_const (ix86_valid_target_attribute)

Message ID 550BF765.2070705@suse.cz
State New
Headers show

Commit Message

Martin Liška March 20, 2015, 10:33 a.m. UTC 
On 03/20/2015 01:10 AM, Jan Hubicka wrote:
>> Hi.
>>
>> Explanation of the patch is introduced.
>
> thanks
>>
>>>>
>>>>         mask &= ~OPTION_MASK_ISA_64BIT;
>>>>         if (mask == 0
>>>> @@ -30670,6 +30673,14 @@ def_builtin_const (HOST_WIDE_INT mask, const char *name,
>>>>   static void
>>>>   ix86_add_new_builtins (HOST_WIDE_INT isa)
>>>>   {
>>>> +  /* Last cached isa value.  */
>>>> +  static HOST_WIDE_INT last_tested_isa_value = 0;
>>>> +
>>>> +  if ((isa & defined_isa_values) == 0 || isa == last_tested_isa_value)
>>>
>>> Heer you need to compare (isa & defined_isa_values) == (isa &
>>> last_tested_isa_value) right, because we have isa flags that enable no
>>> builtins.
>>
>> I do not understand why, the guard simply ignores last value, which is already processed and
>> 'isa' with any intersection with defined_isa_values. Maybe I miss something?
>
> I think you can also skip processing in cases ISA changed but the change has empty intersection with
> defined_isa_values.
>
> Honza
>

Good idea.

I've integrated this part to upgraded patch, which can survive regression tests on x86_64-linux-pc
and can bootstrap.

Ready for trunk?
Thanks,
Martin

Comments

Jakub Jelinek March 20, 2015, 11:24 a.m. UTC | #1 
On Fri, Mar 20, 2015 at 11:33:09AM +0100, Martin Liška wrote:
> @@ -30670,6 +30673,20 @@ def_builtin_const (HOST_WIDE_INT mask, const char *name,
>  static void
>  ix86_add_new_builtins (HOST_WIDE_INT isa)
>  {
> +  /* Last cached isa value.  */
> +  static HOST_WIDE_INT last_tested_isa_value = 0;
> +
> +  /* We iterate through all defined builtins just if the last tested
> +     values is different from ISA and just in case there is any intersection
> +     between ISA value and union of all possible configurations.
> +     Last condition skips iterations if ISA is changed by the change has
> +     empty intersection with defined_isa_values.  */
> +  if ((isa & defined_isa_values) == 0 || isa == last_tested_isa_value
> +       || ((isa ^ last_tested_isa_value) & defined_isa_values) == 0)
> +    return;
> +
> +  last_tested_isa_value = isa;

Isn't at least the isa == last_tested_isa_value test useless?
I mean, if they are equal, then (isa ^ last_tested_isa_value) is 0
and so the third test is true.

Also, given that the loop does something only for
  (ix86_builtins_isa[i].isa & isa) != 0
it means once you call ix86_add_new_builtins with some particular
bit set in the isa, it doesn't make sense to try that bit again.

So, I think it would be better:
1) rename defined_isa_values bitmask to say deferred_isa_values
   (as in, isa bits that might still enable any new builtins)
2) in def_builtin, don't or in the mask unconditionally, but only
   in the else case - when add_builtin_function is not called
3) in ix86_add_new_builtins, don't add last_tested_isa_value at all, instead
   do:
  if ((isa & deferred_isa_values) == 0)
    return;

  deferred_isa_values &= ~isa;

That way, when you actually enable all builtins (either immediately in
def_builtin because it wasn't C/C++-like FE, or because all ISAs were
enabled from the start, or because ix86_add_new_builtins has been already
called with sufficiently full bitmask), deferred_isa_values will be 0 and
you won't do anything in the function any more.

	Jakub
diff mbox

Patch

From 5df909a7d06e91c9506a7d7186015b431f9db04a Mon Sep 17 00:00:00 2001
From: marxin <marxin.liska@gmail.com>
Date: Sun, 8 Mar 2015 19:39:55 -0500
Subject: [PATCH] def_builtin_const: speed-up.

gcc/ChangeLog:

2015-03-09  Martin Liska  <mliska@suse.cz>

	* config/i386/i386.c (def_builtin): Collect union of all
	possible masks.
	(ix86_add_new_builtins): Do not iterate over all builtins
	in cases that isa value has no intersection with possible masks
	and(or) last passed value is equal to the provided.
---
 gcc/config/i386/i386.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ab8f03a..afb2fa4 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -30592,6 +30592,8 @@  struct builtin_isa {
 
 static struct builtin_isa ix86_builtins_isa[(int) IX86_BUILTIN_MAX];
 
+/* Union of all masks that are part of builtin_isa structures.  */
+static HOST_WIDE_INT defined_isa_values = 0;
 
 /* Add an ix86 target builtin function with CODE, NAME and TYPE.  Save the MASK
    of which isa_flags to use in the ix86_builtins_isa array.  Stores the
@@ -30619,6 +30621,7 @@  def_builtin (HOST_WIDE_INT mask, const char *name,
   if (!(mask & OPTION_MASK_ISA_64BIT) || TARGET_64BIT)
     {
       ix86_builtins_isa[(int) code].isa = mask;
+      defined_isa_values |= mask;
 
       mask &= ~OPTION_MASK_ISA_64BIT;
       if (mask == 0
@@ -30670,6 +30673,20 @@  def_builtin_const (HOST_WIDE_INT mask, const char *name,
 static void
 ix86_add_new_builtins (HOST_WIDE_INT isa)
 {
+  /* Last cached isa value.  */
+  static HOST_WIDE_INT last_tested_isa_value = 0;
+
+  /* We iterate through all defined builtins just if the last tested
+     values is different from ISA and just in case there is any intersection
+     between ISA value and union of all possible configurations.
+     Last condition skips iterations if ISA is changed by the change has
+     empty intersection with defined_isa_values.  */
+  if ((isa & defined_isa_values) == 0 || isa == last_tested_isa_value
+       || ((isa ^ last_tested_isa_value) & defined_isa_values) == 0)
+    return;
+
+  last_tested_isa_value = isa;
+
   int i;
   tree saved_current_target_pragma = current_target_pragma;
   current_target_pragma = NULL_TREE;
-- 
2.1.2