diff mbox

[google,4.8] Expose all target specific builtins unconditionally for LIPO builds

Message ID CAAs8Hmz-_Hf8KFSLYO4OppjkVVLeAWU1qmnOF=C=dsXpXfEaPw@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam July 24, 2013, 10:14 p.m. UTC
The following test-case fails in LIPO mode during profile-use build:

main.cc
------------
__attribute__((target("sse4.2")))
unsigned int problem_aux (unsigned int A, unsigned int B);


int main (int argc, char *argv[])
{
  return problem_aux (0, 0);
}

aux.cc
---------
__attribute__((target("sse4.2")))
unsigned int problem_aux (unsigned int A, unsigned int B)
{
  return __builtin_ia32_crc32qi (A, B);
}

$ g++ -O2 -fprofile-generate main.cc aux.cc
$ ./a.out
$ g++ -O2 -fprofile-use main.cc
error: '__builtin_ia32_crc32qi' was not declared in this scope

This happens when parsing the auxiliary module. __builtin_ia32_crc32qi
is a target specific builtin that gets created after the target
attribute is seen in the declaration of problem_aux in the primary
module. This is too late for adding theisbuiltin to the list of
knowns, the buitlins get saved when cp_save_built_in_decl_pre_parsing
in cp/cp-objcp-common.c is called much earlier.  Hence, this builtin
which is created during primary module parsing is not available when
parsing the auxiliary module.

A simple fix is to expose all target builtins unconditionally in LIPO
mode. Patch:




Is this ok?

Thanks
Sri

Comments

Xinliang David Li July 24, 2013, 10:21 p.m. UTC | #1
Can you collect some number on ggc_memory increase  with this change
in profile_gen build -- the value is recorded gcov_module_info object
in coverage.c.

thanks,

David

On Wed, Jul 24, 2013 at 3:14 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> The following test-case fails in LIPO mode during profile-use build:
>
> main.cc
> ------------
> __attribute__((target("sse4.2")))
> unsigned int problem_aux (unsigned int A, unsigned int B);
>
>
> int main (int argc, char *argv[])
> {
>   return problem_aux (0, 0);
> }
>
> aux.cc
> ---------
> __attribute__((target("sse4.2")))
> unsigned int problem_aux (unsigned int A, unsigned int B)
> {
>   return __builtin_ia32_crc32qi (A, B);
> }
>
> $ g++ -O2 -fprofile-generate main.cc aux.cc
> $ ./a.out
> $ g++ -O2 -fprofile-use main.cc
> error: '__builtin_ia32_crc32qi' was not declared in this scope
>
> This happens when parsing the auxiliary module. __builtin_ia32_crc32qi
> is a target specific builtin that gets created after the target
> attribute is seen in the declaration of problem_aux in the primary
> module. This is too late for adding theisbuiltin to the list of
> knowns, the buitlins get saved when cp_save_built_in_decl_pre_parsing
> in cp/cp-objcp-common.c is called much earlier.  Hence, this builtin
> which is created during primary module parsing is not available when
> parsing the auxiliary module.
>
> A simple fix is to expose all target builtins unconditionally in LIPO
> mode. Patch:
>
>
> Index: config/i386/i386.c
> ===================================================================
> --- config/i386/i386.c  (revision 201046)
> +++ config/i386/i386.c  (working copy)
> @@ -26957,7 +26957,8 @@ def_builtin (HOST_WIDE_INT mask, const char *name,
>        ix86_builtins_isa[(int) code].isa = mask;
>
>        mask &= ~OPTION_MASK_ISA_64BIT;
> -      if (mask == 0
> +      if (flag_dyn_ipa
> +         || mask == 0
>           || (mask & ix86_isa_flags) != 0
>           || (lang_hooks.builtin_function
>               == lang_hooks.builtin_function_ext_scope))
>
>
> Is this ok?
>
> Thanks
> Sri
Sriraman Tallam July 25, 2013, 9:53 p.m. UTC | #2
On Wed, Jul 24, 2013 at 3:21 PM, Xinliang David Li <davidxl@google.com> wrote:
> Can you collect some number on ggc_memory increase  with this change
> in profile_gen build -- the value is recorded gcov_module_info object
> in coverage.c.

The ggc_memory increase if < 0.2% for large modules (initial usage
around 100MB and greater).  For some small modules, whose initial
parser memory usage is around 2MB, I see a 80% increase.

Thanks
Sri

>
> thanks,
>
> David
>
> On Wed, Jul 24, 2013 at 3:14 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> The following test-case fails in LIPO mode during profile-use build:
>>
>> main.ccthe
>> ------------
>> __attribute__((target("sse4.2")))
>> unsigned int problem_aux (unsigned int A, unsigned int B);
>>
>>
>> int main (int argc, char *argv[])
>> {
>>   return problem_aux (0, 0);
>> }
>>
>> aux.cc
>> ---------
>> __attribute__((target("sse4.2")))
>> unsigned int problem_aux (unsigned int A, unsigned int B)
>> {
>>   return __builtin_ia32_crc32qi (A, B);
>> }
>>
>> $ g++ -O2 -fprofile-generate main.cc aux.cc
>> $ ./a.out
>> $ g++ -O2 -fprofile-use main.cc
>> error: '__builtin_ia32_crc32qi' was not declared in this scope
>>
>> This happens when parsing the auxiliary module. __builtin_ia32_crc32qi
>> is a target specific builtin that gets created after the target
>> attribute is seen in the declaration of problem_aux in the primary
>> module. This is too late for adding theisbuiltin to the list of
>> knowns, the buitlins get saved when cp_save_built_in_decl_pre_parsing
>> in cp/cp-objcp-common.c is called much earlier.  Hence, this builtin
>> which is created during primary module parsing is not available when
>> parsing the auxiliary module.
>>
>> A simple fix is to expose all target builtins unconditionally in LIPO
>> mode. Patch:
>>
>>
>> Index: config/i386/i386.c
>> ===================================================================
>> --- config/i386/i386.c  (revision 201046)
>> +++ config/i386/i386.c  (working copy)
>> @@ -26957,7 +26957,8 @@ def_builtin (HOST_WIDE_INT mask, const char *name,
>>        ix86_builtins_isa[(int) code].isa = mask;
>>
>>        mask &= ~OPTION_MASK_ISA_64BIT;
>> -      if (mask == 0
>> +      if (flag_dyn_ipa
>> +         || mask == 0
>>           || (mask & ix86_isa_flags) != 0
>>           || (lang_hooks.builtin_function
>>               == lang_hooks.builtin_function_ext_scope))
>>
>>
>> Is this ok?
>>
>> Thanks
>> Sri
Xinliang David Li July 25, 2013, 10:02 p.m. UTC | #3
the patch is ok.

thanks,

David

On Thu, Jul 25, 2013 at 2:53 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Wed, Jul 24, 2013 at 3:21 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Can you collect some number on ggc_memory increase  with this change
>> in profile_gen build -- the value is recorded gcov_module_info object
>> in coverage.c.
>
> The ggc_memory increase if < 0.2% for large modules (initial usage
> around 100MB and greater).  For some small modules, whose initial
> parser memory usage is around 2MB, I see a 80% increase.
>
> Thanks
> Sri
>
>>
>> thanks,
>>
>> David
>>
>> On Wed, Jul 24, 2013 at 3:14 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> The following test-case fails in LIPO mode during profile-use build:
>>>
>>> main.ccthe
>>> ------------
>>> __attribute__((target("sse4.2")))
>>> unsigned int problem_aux (unsigned int A, unsigned int B);
>>>
>>>
>>> int main (int argc, char *argv[])
>>> {
>>>   return problem_aux (0, 0);
>>> }
>>>
>>> aux.cc
>>> ---------
>>> __attribute__((target("sse4.2")))
>>> unsigned int problem_aux (unsigned int A, unsigned int B)
>>> {
>>>   return __builtin_ia32_crc32qi (A, B);
>>> }
>>>
>>> $ g++ -O2 -fprofile-generate main.cc aux.cc
>>> $ ./a.out
>>> $ g++ -O2 -fprofile-use main.cc
>>> error: '__builtin_ia32_crc32qi' was not declared in this scope
>>>
>>> This happens when parsing the auxiliary module. __builtin_ia32_crc32qi
>>> is a target specific builtin that gets created after the target
>>> attribute is seen in the declaration of problem_aux in the primary
>>> module. This is too late for adding theisbuiltin to the list of
>>> knowns, the buitlins get saved when cp_save_built_in_decl_pre_parsing
>>> in cp/cp-objcp-common.c is called much earlier.  Hence, this builtin
>>> which is created during primary module parsing is not available when
>>> parsing the auxiliary module.
>>>
>>> A simple fix is to expose all target builtins unconditionally in LIPO
>>> mode. Patch:
>>>
>>>
>>> Index: config/i386/i386.c
>>> ===================================================================
>>> --- config/i386/i386.c  (revision 201046)
>>> +++ config/i386/i386.c  (working copy)
>>> @@ -26957,7 +26957,8 @@ def_builtin (HOST_WIDE_INT mask, const char *name,
>>>        ix86_builtins_isa[(int) code].isa = mask;
>>>
>>>        mask &= ~OPTION_MASK_ISA_64BIT;
>>> -      if (mask == 0
>>> +      if (flag_dyn_ipa
>>> +         || mask == 0
>>>           || (mask & ix86_isa_flags) != 0
>>>           || (lang_hooks.builtin_function
>>>               == lang_hooks.builtin_function_ext_scope))
>>>
>>>
>>> Is this ok?
>>>
>>> Thanks
>>> Sri
diff mbox

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c  (revision 201046)
+++ config/i386/i386.c  (working copy)
@@ -26957,7 +26957,8 @@  def_builtin (HOST_WIDE_INT mask, const char *name,
       ix86_builtins_isa[(int) code].isa = mask;

       mask &= ~OPTION_MASK_ISA_64BIT;
-      if (mask == 0
+      if (flag_dyn_ipa
+         || mask == 0
          || (mask & ix86_isa_flags) != 0
          || (lang_hooks.builtin_function
              == lang_hooks.builtin_function_ext_scope))