> On Oct 18, 2021, at 10:36 AM, Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Mon, Oct 18, 2021 at 03:04:40PM +0000, Qing Zhao wrote:
>> 2021-10-16 qing zhao <qing.z...@oracle.com>
>>
>> * gimplify.c (gimplify_decl_expr): Do not add call to
>> __BUILTIN_CLEAR_PADDING when a variable is a gimple register
>
> The builtin is called __builtin_clear_padding, using __BUILTIN_CLEAR_PADDING
> makes no sense, either use the lower-case version of it, or use
> BUILT_IN_CLEAR_PADDING which is the enum built_in_function enumerator
> used to refer to it in the compiler.
Okay, will fix all the names in the patch.
>
>> --- a/gcc/gimplify.c
>> +++ b/gcc/gimplify.c
>> @@ -1954,8 +1954,14 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>> pattern initialization.
>> In order to make the paddings as zeroes for pattern init, We
>> should add a call to __builtin_clear_padding to clear the
>> - paddings to zero in compatiple with CLANG. */
>> - if (flag_auto_var_init == AUTO_INIT_PATTERN)
>> + paddings to zero in compatiple with CLANG.
>> + We cannot insert this call if the variable is a gimple register
>> + since __BUILTIN_CLEAR_PADDING assumes the variable is in memory.
>
> Likewise (and please fix the other spots in gimplify.c that do that too.
Yes, will fix that.
> Furthermore, __builtin_clear_padding doesn't assume anything, but it takes
> an address of an object as argument and already the taking of the address
> that gimple_add_padding_init_for_auto_var does makes the var
> TREE_ADDRESABLE, which is something that needs to be done before the var is
> ever accessed during gimplification.
Yes, currently, “gimple_add_padding_init_for_auto_var” has already done the
following before
calling __builtin_clear_padding:
mark_addressable (decl);
addr_of_decl = build_fold_addr_expr (decl);
But looks like that “making the DECL as ADDRESSABLE here” is too late.
Yes, If we can “make the DECL as ADDRESSABLE” before the var is accessed during
gimplification, that will also
fix this issue. But “Where” is the right place to make the DECL as ADDRESSABLE?
(Do it in “gimplify_decl_expr” when handling
The variable’s declaration? )
>
>> + As a result, if a long double/Complex long double variable will
>
> Please change Complex to _Complex
Okay.
>
>> + spilled into stack later, its padding is 0XFE. */
>> + if (flag_auto_var_init == AUTO_INIT_PATTERN
>> + && !is_gimple_reg (decl)
>> + && clear_padding_type_may_have_padding_p (TREE_TYPE (decl)))
>> gimple_add_padding_init_for_auto_var (decl, is_vla, seq_p);
>> }
>> }
>> @@ -5388,8 +5394,15 @@ gimplify_init_constructor (tree *expr_p, gimple_seq
>> *pre_p, gimple_seq *post_p,
>> initialize paddings of object always to zero regardless of
>> INIT_TYPE. Note, we will not insert this call if the aggregate
>> variable has be completely cleared already or it's initialized
>> - with an empty constructor. */
>> + with an empty constructor. We cannot insert this call if the
>> + variable is a gimple register since __BUILTIN_CLEAR_PADDING assumes
>> + the variable is in memory. As a result, if a long double/Complex long
>
> Likewise.
Okay.
>> + double variable will be spilled into stack later, its padding cannot
>> + be cleared with __BUILTIN_CLEAR_PADDING. we should clear its padding
>
> s/we/We/
Okay.
>
>> --- /dev/null
>> +++ b/gcc/testsuite/c-c++-common/pr102281.c
>> @@ -0,0 +1,15 @@
>> +/* PR102281 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-ftrivial-auto-var-init=zero" } */
>> +long _mm_set_epi64x___q0;
>> +__attribute__((__vector_size__(2 * sizeof(long)))) long long
>> _mm_set_epi64x() {
>> + return (__attribute__((__vector_size__(2 * sizeof(long)))) long long){
>> + _mm_set_epi64x___q0};
>> +}
>> +
>> +float _mm_set1_ps___F;
>> +__attribute__((__vector_size__(4 * sizeof(float)))) float
>> +__attribute___mm_set1_ps() {
>> + return (__attribute__((__vector_size__(4 * sizeof(float)))) float){
>> + _mm_set1_ps___F};
>> +}
>
> If it is a generic testcase, please change the variable and function names
> so that they don't look like x86 intrinsics, using v, w or var1, var2 etc.
> instead of _mm_set_epi64x___q0 or _mm_set1_ps___F, and
> foo/bar instead of _mm_set_epi64x or __attribute___mm_set1_ps will make it
> certainly more readable. Please add typedefs for the vector types,
> typedef long long V __attribute__((__vector_size__ (2 * sizeof (long long))));
> typedef float W __attribute__((__vector_size__ (4 * sizeof (float))));
> and use that (note that I've changed the long in there to long long to make
> it match.
> And please use (void) instead of () in the function declarations.
> Also, the testcase will likely fail on x86-64 with
> RUNTESTFLAGS='--target_board=unix\{-m32/-mno-sse,-m32/-msse2,-m64\}
> dg.exp=pr102281.c'
> because of -Wpsabi warnings. So, you likely want -Wno-psabi or even
> -Wno-psabi -w
> in dg-options.
Thanks for the detailed suggestions, I will fix this testing case.
Qing
>
> Jakub
>
