Speed-up def_builtin_const (ix86_valid_target_attribute)

On 03/20/2015 12:24 PM, Jakub Jelinek wrote:
> 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
>

Thank you Jakub for smart solution.
I've just implemented new patch, which I've been testing.

What do you think about it?
Martin

> gcc/ChangeLog:
> 
> 2015-03-20  Martin Liska  <mliska@suse.cz>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
> 	* config/i386/i386.c (def_builtin): Set deferred_isa_values for
> 	masks that can potentially include a builtin.
> 	(ix86_add_new_builtins): Introduce fast filter for isa values
> 	that cannot trigger builtin inclusion.

Thanks, this looks better.
Patch is OK if it passes testing.

Honza
> ---
>  gcc/config/i386/i386.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 47deda7..82a4848 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -30588,6 +30588,8 @@ struct builtin_isa {
>  
>  static struct builtin_isa ix86_builtins_isa[(int) IX86_BUILTIN_MAX];
>  
> +/* Bits that can still enable any inclusion of a builtin.  */
> +static HOST_WIDE_INT deferred_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
> @@ -30631,6 +30633,9 @@ def_builtin (HOST_WIDE_INT mask, const char *name,
>  	}
>        else
>  	{
> +	  /* Just a MASK where set_and_not_built_p == true can potentially
> +	     include a builtin.  */
> +	  deferred_isa_values |= mask;
>  	  ix86_builtins[(int) code] = NULL_TREE;
>  	  ix86_builtins_isa[(int) code].tcode = tcode;
>  	  ix86_builtins_isa[(int) code].name = name;
> @@ -30666,6 +30671,12 @@ def_builtin_const (HOST_WIDE_INT mask, const char *name,
>  static void
>  ix86_add_new_builtins (HOST_WIDE_INT isa)
>  {
> +  if ((isa & deferred_isa_values) == 0)
> +    return;
> +
> +  /* Bits in ISA value can be removed from potential isa values.  */
> +  deferred_isa_values &= ~isa;
> +
>    int i;
>    tree saved_current_target_pragma = current_target_pragma;
>    current_target_pragma = NULL_TREE;
> -- 
> 2.1.2
>