GCC does not support *mmintrin.h with function specific opts

Hi Jakub,

   I have taken your proposed changes and made patch for this.  Please
let me know what you think. I have changed only the headers mmintrin.h
and x86intrin.h as that includes all the other headers.  The builtins
get enabled automatically when the pragma target is specified so need
to do any thing to def_builtin.  I have included 4 test case, where
intrinsics_4.c uses your example with __mm256_and_ps. I had to fix a
bug with lzcnt builtins in i386-common.c as that was not handled
there.

Thanks
Sri

On Wed, May 15, 2013 at 7:25 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, May 14, 2013 at 3:04 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Tue, May 14, 2013 at 10:39:13AM +0200, Jakub Jelinek wrote:
>>> When trying with -O2 -mno-avx:
>>> #ifndef __AVX__
>>> #pragma GCC push_options
>>> #pragma GCC target("avx")
>>> #define __DISABLE_AVX__
>>> #endif
>>> typedef float __v8sf __attribute__ ((__vector_size__ (32)));
>>> typedef float __m256 __attribute__ ((__vector_size__ (32), __may_alias__));
>>> extern __inline __m256 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
>>> _mm256_and_ps (__m256 __A, __m256 __B) { return (__m256) __builtin_ia32_andps256 ((__v8sf)__A, (__v8sf)__B); }
>>> #ifdef __DISABLE_AVX__
>>> #pragma GCC pop_options
>>> #undef __DISABLE_AVX__
>>> #endif
>>> __m256 a, b, c;
>>> void __attribute__((target ("avx")))
>>> foo (void)
>>> {
>>>   a = _mm256_and_ps (b, c);
>>> }
>>> we get bogus errors and ICE:
>>> tty2.c: In function '_mm256_and_ps':
>>> tty2.c:9:1: note: The ABI for passing parameters with 32-byte alignment has changed in GCC 4.6
>>> tty2.c: In function 'foo':
>>> tty2.c:9:82: error: '__builtin_ia32_andps256' needs isa option -m32
>>> tty2.c:9:82: internal compiler error: in emit_move_insn, at expr.c:3486
>>> 0x77a3d2 emit_move_insn(rtx_def*, rtx_def*)
>>>       ../../gcc/expr.c:3485
>>> (I have added "1 ||" instead of your generate_builtins into i386.c
>>> (def_builtin)), that just shows that target attribute/pragma support still
>>> has very severe issues that need to be fixed, instead of papered around.
>>>
>>> Note, we ICE on:
>>> #pragma GCC target ("mavx")
>>> That should be fixed too.
>>
>> Ok, I had a brief look at the above two issues.
>>
>> The first testcase has the problem that the ix86_previous_fndecl cache
>> gets out of date.  When set_cfun is called on _mm256_and_ps (with the
>> implicit avx attribute), then ix86_previous_fndecl is set to _mm256_and_ps,
>> TARGET_AVX is set to true, target reinited.  Then set_cfun is called
>> with NULL, we don't do anything.  Later on #pragma GCC pop_options appears,
>> sets !TARGET_AVX (as that is the new target_option_current_node).
>> Next foo is being parsed, avx attribute is noticed, the same target node
>> is used for it, but when set_cfun is called for foo, ix86_previous_fndecl's
>> target node is the same as foo's and so we don't do cl_target_restore_option
>> at all, so !TARGET_AVX remains, while it should be set.  That is the reason
>> for the bogus inform etc.  Fixed by resetting the ix86_previous_fndecl cache
>> on any #pragma GCC target below.  The #pragma GCC target ("mavx") is also
>> fixed below.  The patch also includes the "1 ||" to enable building all
>> builtins.  We still ICE with:
>> #0  fancy_abort (file=0x11d8fad "../../gcc/expr.c", line=316, function=0x11dada3 "convert_move") at ../../gcc/diagnostic.c:1180
>> #1  0x0000000000771c39 in convert_move (to=0x7ffff1b2df00, from=0x7ffff1b314e0, unsignedp=0) at ../../gcc/expr.c:316
>> #2  0x000000000078009f in store_expr (exp=0x7ffff19ab390, target=0x7ffff1b2df00, call_param_p=0, nontemporal=false) at ../../gcc/expr.c:5300
>> #3  0x000000000077eba1 in expand_assignment (to=0x7ffff1b35090, from=0x7ffff19ab390, nontemporal=false) at ../../gcc/expr.c:5025
>> on the first testcase.  We don't ICE say on:
>> #ifndef __AVX__
>> #pragma GCC push_options
>> #pragma GCC target("avx")
>> #define __DISABLE_AVX__
>> #endif
>> typedef float __v8sf __attribute__ ((__vector_size__ (32)));
>> typedef float __m256 __attribute__ ((__vector_size__ (32), __may_alias__));
>> extern __inline __m256 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
>> _mm256_and_ps (__m256 __A, __m256 __B) { return (__m256) __builtin_ia32_andps256 ((__v8sf)__A, (__v8sf)__B); }
>> #ifdef __DISABLE_AVX__
>> #pragma GCC pop_options
>> #undef __DISABLE_AVX__
>> #endif
>> __m256 a[10], b[10], c[10];
>> void __attribute__((target ("avx")))
>> foo (void)
>> {
>>   a[0] = _mm256_and_ps (b[0], c[0]);
>> }
>> The problem is that in the first testcase, the VAR_DECL c (guess also b and
>> a) have TYPE_MODE (TREE_TYPE (c)) == V8SFmode (this is dynamic, for vector
>> types TYPE_MODE is a function call), but DECL_MODE (c) is BLKmode
>> (it has been laid out while -mno-avx has been the current) and also
>> DECL_RTL which is a mem:BLK.  Guess expr.c would need to special case
>> TREE_STATIC or DECL_EXTERNAL VAR_DECLs with vector type, if they have
>> DECL_MODE BLKmode, but TYPE_MODE some vector type, just adjust the MEM
>> to the desired mode?
>>
>> --- gcc/config/i386/i386-c.c.jj 2013-01-15 17:20:37.000000000 +0100
>> +++ gcc/config/i386/i386-c.c    2013-05-14 11:46:50.773806894 +0200
>> @@ -369,20 +369,23 @@ ix86_pragma_target_parse (tree args, tre
>>
>>    if (! args)
>>      {
>> -      cur_tree = ((pop_target)
>> -                 ? pop_target
>> -                 : target_option_default_node);
>> +      cur_tree = (pop_target ? pop_target : target_option_default_node);
>>        cl_target_option_restore (&global_options,
>>                                 TREE_TARGET_OPTION (cur_tree));
>>      }
>>    else
>>      {
>>        cur_tree = ix86_valid_target_attribute_tree (args);
>> -      if (!cur_tree)
>> -       return false;
>> +      if (!cur_tree || cur_tree == error_mark_node)
>> +       {
>> +         cl_target_option_restore (&global_options,
>> +                                   TREE_TARGET_OPTION (prev_tree));
>> +         return false;
>> +       }
>>      }
>>
>>    target_option_current_node = cur_tree;
>> +  ix86_reset_previous_fndecl ();
>>
>>    /* Figure out the previous/current isa, arch, tune and the differences.  */
>>    prev_opt  = TREE_TARGET_OPTION (prev_tree);
>> --- gcc/config/i386/i386-protos.h.jj    2013-04-03 08:28:50.000000000 +0200
>> +++ gcc/config/i386/i386-protos.h       2013-05-14 11:41:35.389638299 +0200
>> @@ -40,6 +40,8 @@ extern void ix86_output_addr_diff_elt (F
>>  extern enum calling_abi ix86_cfun_abi (void);
>>  extern enum calling_abi ix86_function_type_abi (const_tree);
>>
>> +extern void ix86_reset_previous_fndecl (void);
>> +
>>  #ifdef RTX_CODE
>>  extern int standard_80387_constant_p (rtx);
>>  extern const char *standard_80387_constant_opcode (rtx);
>> --- gcc/config/i386/i386.c.jj   2013-05-14 08:23:31.000000000 +0200
>> +++ gcc/config/i386/i386.c      2013-05-14 11:40:54.996867411 +0200
>> @@ -4559,6 +4559,13 @@ ix86_can_inline_p (tree caller, tree cal
>>  /* Remember the last target of ix86_set_current_function.  */
>>  static GTY(()) tree ix86_previous_fndecl;
>>
>> +/* Invalidate ix86_previous_fndecl cache.  */
>> +void
>> +ix86_reset_previous_fndecl (void)
>> +{
>> +  ix86_previous_fndecl = NULL_TREE;
>> +}
>> +
>>  /* Establish appropriate back-end context for processing the function
>>     FNDECL.  The argument might be NULL to indicate processing at top
>>     level, outside of any function scope.  */
>> @@ -26829,7 +26836,7 @@ def_builtin (HOST_WIDE_INT mask, const c
>>        ix86_builtins_isa[(int) code].isa = mask;
>>
>>        mask &= ~OPTION_MASK_ISA_64BIT;
>> -      if (mask == 0
>> +      if (/* HACK */ 1 || mask == 0
>>           || (mask & ix86_isa_flags) != 0
>>           || (lang_hooks.builtin_function
>>               == lang_hooks.builtin_function_ext_scope))
>
> This does not seem necessary now once the #pragma GCC target is used
> in the header. The target-specific builtins seem to be generated when
> the target is specified using pragmas.
>
> I will consolidate your patches and send one shortly.
>
> Thanks for clearing this,
> Sri
>
>
>>
>>
>>         Jakub
This patch allows *intrin.h headers to be used with function target attributes.
To do that, either include x86intrin.h or immintrin.h as they have guards to
enable other headers when the target options are not passed via the command
line.  

	* testsuite/gcc.target/i386/intrinsics_4.c: New test.
	* testsuite/gcc.target/i386/intrinsics_1.c: New test.
	* testsuite/gcc.target/i386/intrinsics_2.c: Ditto.
	* testsuite/gcc.target/i386/intrinsics_3.c: Ditto.
	* config/i386/x86intrin.h: Guard every header inclusion by pushing the right
	target attribute when the target is not defined.
	* config/i386/immintrin.h: Guard every header inclusion by pushing the right
	target attribute when the target is not defined.  Include
	popcntintrin.h also here.
	* config/i386/i386.c (ix86_pragma_target_parse):  Restore target
	when current target options does not apply.
	* config/i386/i386-protos.h (ix86_reset_previous_fndecl): New function.
	* config/i386/i386.c (ix86_reset_previous_fndecl): Ditto.
	* common/config/i386/i386-common.c: Handle LZCNT.

GCC does not support *mmintrin.h with function specific opts

Commit Message

Comments

Patch