From: Richard Biener <richard.guenther@gmail.com>
To: Qing Zhao <qing.zhao@oracle.com>
Cc: Jakub Jelinek <jakub@redhat.com>,
gcc-patches Nick Alcock via <gcc-patches@gcc.gnu.org>,
"Kewen. Lin" <linkw@linux.ibm.com>,
"H.J. Lu" <hjl.tools@gmail.com>,
andrew Pinski <pinskia@gmail.com>
Subject: Re: Values of WIDE_INT_MAX_ELTS in gcc11 and gcc12 are different
Date: Wed, 10 Nov 2021 09:37:52 +0100 [thread overview]
Message-ID: <CAFiYyc2Gsaq7qzUns-kzkA0kZ4_bJZfesiGm+yFz9X-L7wCxHw@mail.gmail.com> (raw)
In-Reply-To: <3DEDC02A-FD41-4683-8CCA-F00ACD25E70A@oracle.com>
On Tue, Nov 9, 2021 at 6:48 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>
> So, based on the discussion so far, is the following patch good to go?
OK.
Thanks,
Richard.
> Let me know if you have more comments on the following patch:
>
> (At the same time, I am testing this patch on both x86 and aarch64)
>
> thanks.
>
> Qing
>
> diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
> index 0cba95411a6..e8fd16b9c21 100644
> --- a/gcc/internal-fn.c
> +++ b/gcc/internal-fn.c
> @@ -3059,10 +3059,10 @@ expand_DEFERRED_INIT (internal_fn, gcall *stmt)
> mark_addressable (lhs);
> tree var_addr = build_fold_addr_expr (lhs);
>
> - tree value = (init_type == AUTO_INIT_PATTERN) ?
> - build_int_cst (integer_type_node,
> - INIT_PATTERN_VALUE) :
> - integer_zero_node;
> + tree value = (init_type == AUTO_INIT_PATTERN)
> + ? build_int_cst (integer_type_node,
> + INIT_PATTERN_VALUE)
> + : integer_zero_node;
> tree m_call = build_call_expr (builtin_decl_implicit (BUILT_IN_MEMSET),
> 3, var_addr, value, var_size);
> /* Expand this memset call. */
> @@ -3073,15 +3073,17 @@ expand_DEFERRED_INIT (internal_fn, gcall *stmt)
> /* If this variable is in a register use expand_assignment.
> For boolean scalars force zero-init. */
> tree init;
> + scalar_int_mode var_mode;
> if (TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE
> && tree_fits_uhwi_p (var_size)
> && (init_type == AUTO_INIT_PATTERN
> || !is_gimple_reg_type (var_type))
> && int_mode_for_size (tree_to_uhwi (var_size) * BITS_PER_UNIT,
> - 0).exists ())
> + 0).exists (&var_mode)
> + && have_insn_for (SET, var_mode))
> {
> unsigned HOST_WIDE_INT total_bytes = tree_to_uhwi (var_size);
> - unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
> + unsigned char *buf = XALLOCAVEC (unsigned char, total_bytes);
> memset (buf, (init_type == AUTO_INIT_PATTERN
> ? INIT_PATTERN_VALUE : 0), total_bytes);
> tree itype = build_nonstandard_integer_type
> diff --git a/gcc/testsuite/gcc.target/i386/auto-init-6.c b/gcc/testsuite/gcc.target/i386/auto-init-6.c
> index 339f8bc2966..e53385f0eb7 100644
> --- a/gcc/testsuite/gcc.target/i386/auto-init-6.c
> +++ b/gcc/testsuite/gcc.target/i386/auto-init-6.c
> @@ -1,4 +1,6 @@
> /* Verify pattern initialization for complex type automatic variables. */
> +/* Note, _Complex long double is initialized to zeroes due to the current
> + implemenation limitation. */
> /* { dg-do compile } */
> /* { dg-options "-ftrivial-auto-var-init=pattern -march=x86-64 -mtune=generic -msse" } */
>
> @@ -15,6 +17,6 @@ _Complex long double foo()
> return result;
> }
>
> -/* { dg-final { scan-assembler-times "long\t-16843010" 10 { target { ! ia32 } } } } */
> -/* { dg-final { scan-assembler-times "long\t-16843010" 6 { target { ia32 } } } } */
> +/* { dg-final { scan-assembler-times "long\t0" 8 { target { ! ia32 } } } } */
> +/* { dg-final { scan-assembler-times "long\t-16843010" 6 } } */
>
>
>
>
> > On Nov 9, 2021, at 4:44 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Nov 9, 2021 at 10:10 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >>
> >> On Tue, Nov 09, 2021 at 08:13:57AM +0100, Richard Biener wrote:
> >>>> Hi, I tried both the following patches:
> >>>>
> >>>> Patch1:
> >>>>
> >>>> [opc@qinzhao-ol8u3-x86 gcc]$ git diff
> >>>> diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
> >>>> index 0cba95411a6..ca49d2b4514 100644
> >>>> --- a/gcc/internal-fn.c
> >>>> +++ b/gcc/internal-fn.c
> >>>> @@ -3073,12 +3073,14 @@ expand_DEFERRED_INIT (internal_fn, gcall *stmt)
> >>>> /* If this variable is in a register use expand_assignment.
> >>>> For boolean scalars force zero-init. */
> >>>> tree init;
> >>>> + scalar_int_mode var_mode;
> >>>> if (TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE
> >>>> && tree_fits_uhwi_p (var_size)
> >>>> && (init_type == AUTO_INIT_PATTERN
> >>>> || !is_gimple_reg_type (var_type))
> >>>> && int_mode_for_size (tree_to_uhwi (var_size) * BITS_PER_UNIT,
> >>>> - 0).exists ())
> >>>> + 0).exists (&var_mode)
> >>>> + && targetm.scalar_mode_supported_p (var_mode))
> >>>> {
> >>>> unsigned HOST_WIDE_INT total_bytes = tree_to_uhwi (var_size);
> >>>> unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
> >>>>
> >>>> AND
> >>>>
> >>>> Patch2:
> >>>> diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
> >>>> index 0cba95411a6..7f129655926 100644
> >>>> --- a/gcc/internal-fn.c
> >>>> +++ b/gcc/internal-fn.c
> >>>> @@ -3073,12 +3073,14 @@ expand_DEFERRED_INIT (internal_fn, gcall *stmt)
> >>>> /* If this variable is in a register use expand_assignment.
> >>>> For boolean scalars force zero-init. */
> >>>> tree init;
> >>>> + scalar_int_mode var_mode;
> >>>> if (TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE
> >>>> && tree_fits_uhwi_p (var_size)
> >>>> && (init_type == AUTO_INIT_PATTERN
> >>>> || !is_gimple_reg_type (var_type))
> >>>> && int_mode_for_size (tree_to_uhwi (var_size) * BITS_PER_UNIT,
> >>>> - 0).exists ())
> >>>> + 0).exists (&var_mode)
> >>>> + && have_insn_for (SET, var_mode))
> >>>> {
> >>>> unsigned HOST_WIDE_INT total_bytes = tree_to_uhwi (var_size);
> >>>> unsigned char *buf = (unsigned char *) xmalloc (total_bytes);
> >>>>
> >>>> Have the same effect:
> >>>>
> >>>> 1. Resolved the ICE in gcc11;
> >>>> 2. For _Complex long double variables, both return FALSE, as a result, for PATTERN initialization of _Complex long double variables, now they are initialization with ZEROs instead of FEs.
> >>>>
> >>>> Let me know you opinion on this, If the above 2 is okay, then I might pick the above Patch 1 for the final patch to this issue.
> >>>
> >>> I think zero-initialization is OK, but I'd choose Patch2 for
> >>> consistency with what we do in the memcpy
> >>> folding.
> >>
> >> Note, I think the code leaks memory (buf is never freed) and
> >> should be allocated using
> >> unsigned char *buf = XNEWVEC (unsigned char *, total_bytes);
> >> and deallocated with XDELETEVEC (buf);
> >
> > Oops. I think the intention was to use XALLOCAVEC
> >
> >> If lhs is SSA_NAME, I think it would be easiest to use
> >> native_interpret_expr directly instead of finding some integral
> >> mode, after memset just native_interpret_expr.
> >
> > I've removed the native_interpret_expr path, I think it would be best to not
> > rely on expand_assignment for the case of a register destination. The
> > intent is to do an integer move into the destination and avoid issues with
> > things like XFmode or other modes with padding (and possibly trapping loads).
> > So it should become some
> >
> > (set (subreg:IntMode ...) (const_wide_int ...))
> >
> > but my RTL fu is too weak to try and so we ended up with expand_assignment
> > which doesn't like V_C_E on SSA names on the LHS (well, because that's
> > not valid GIMPLE...).
> >
> >> Should be guarded by
> >> BITS_PER_UNIT == 8 && CHAR_BIT == 8 && BYTES_BIG_ENDIAN == WORDS_BIG_ENDIAN
> >> but I think the current code assumes at least that BITS_PER_UNIT == CHAR_BIT
> >> too anyway.
> >> That way you get directly a constant init instead of hopping through
> >> some integral mode which might not exist at all.
> >> If lhs is not SSA_NAME but var_type has COMPLEX_TYPE or VECTOR_TYPE, there
> >> is always an option to find var_mode only for the element type (i.e.
> >> TREE_TYPE (var_type), use var_size for the element size too, and after
> >> you compute init fold_build2 (COMPLEX_EXPR, ) or build_vector_from_val.
> >> For VECTOR_TYPE one would need to check if the vector mode is supported,
> >> sure (but if it isn't, it will go the BUILT_IN_MEMSET way, I'm pretty sure).
> >>
> >> Also,
> >> tree value = (init_type == AUTO_INIT_PATTERN) ?
> >> build_int_cst (integer_type_node,
> >> INIT_PATTERN_VALUE) :
> >> integer_zero_node;
> >> a few lines before has bad formatting, both ? and : should be at the start
> >> of next lines rather than on end of the previous ones.
> >>
> >> Jakub
>
next prev parent reply other threads:[~2021-11-10 8:38 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-04 20:37 Qing Zhao
2021-11-05 1:34 ` Kewen.Lin
2021-11-05 6:05 ` Andrew Pinski
2021-11-05 6:53 ` Jakub Jelinek
2021-11-05 10:01 ` Richard Biener
2021-11-05 12:25 ` H.J. Lu
2021-11-05 16:11 ` Qing Zhao
2021-11-05 16:17 ` Jakub Jelinek
2021-11-05 17:37 ` Qing Zhao
2021-11-06 9:56 ` Jakub Jelinek
2021-11-08 8:41 ` Richard Biener
2021-11-08 23:47 ` Qing Zhao
2021-11-09 7:13 ` Richard Biener
2021-11-09 9:10 ` Jakub Jelinek
2021-11-09 10:44 ` Richard Biener
2021-11-09 17:19 ` Qing Zhao
2021-11-10 8:37 ` Richard Biener [this message]
2021-11-10 18:02 ` Qing Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc2Gsaq7qzUns-kzkA0kZ4_bJZfesiGm+yFz9X-L7wCxHw@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
--cc=jakub@redhat.com \
--cc=linkw@linux.ibm.com \
--cc=pinskia@gmail.com \
--cc=qing.zhao@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).