public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Sandiford <richard.sandiford@arm.com>
To: Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org>
Cc: liuhongt <hongtao.liu@intel.com>,
	 Richard Biener <richard.guenther@gmail.com>,
	 crazylht@gmail.com,  hjl.tools@gmail.com
Subject: Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
Date: Wed, 21 Jun 2023 10:32:28 +0100	[thread overview]
Message-ID: <mptttv188k3.fsf@arm.com> (raw)
In-Reply-To: <mpt4jn19o4d.fsf@arm.com> (Richard Sandiford's message of "Wed, 21 Jun 2023 10:10:58 +0100")

Richard Sandiford <richard.sandiford@arm.com> writes:
> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> We have already use intermidate type in case WIDEN, but not for NONE,
>>> this patch extended that.
>>>
>>> I didn't do that in pattern recog since we need to know whether the
>>> stmt belongs to any slp_node to decide the vectype, the related optabs
>>> are checked according to vectype_in and vectype_out. For non-slp case,
>>> vec_pack/unpack are always used when lhs has different size from rhs,
>>> for slp case, sometimes vec_pack/unpack is used, somethings
>>> direct conversion is used.
>>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
>>> Ok for trunk?
>>>
>>> gcc/ChangeLog:
>>>
>>>         PR target/110018
>>>         * tree-vect-stmts.cc (vectorizable_conversion): Use
>>>         intermiediate integer type for float_expr/fix_trunc_expr when
>>>         direct optab is not existed.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>         * gcc.target/i386/pr110018-1.c: New test.
>>> ---
>>>  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
>>>  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
>>>  2 files changed, 149 insertions(+), 1 deletion(-)
>>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
>>>
>>> diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>>> new file mode 100644
>>> index 00000000000..b1baffd7af1
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>>> @@ -0,0 +1,94 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
>>> +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
>>> +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
>>> +
>>> +void
>>> +foo (double* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo1 (float* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> +
>>> +void
>>> +foo2 (_Float16* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +  a[4] = b[4];
>>> +  a[5] = b[5];
>>> +  a[6] = b[6];
>>> +  a[7] = b[7];
>>> +}
>>> +
>>> +void
>>> +foo3 (double* __restrict a, short* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo4 (float* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> +
>>> +void
>>> +foo5 (double* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo6 (float* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> +
>>> +void
>>> +foo7 (_Float16* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +  a[4] = b[4];
>>> +  a[5] = b[5];
>>> +  a[6] = b[6];
>>> +  a[7] = b[7];
>>> +}
>>> +
>>> +void
>>> +foo8 (double* __restrict b, short* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo9 (float* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
>>> index bd3b07a3aa1..1118c89686d 100644
>>> --- a/gcc/tree-vect-stmts.cc
>>> +++ b/gcc/tree-vect-stmts.cc
>>> @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
>>>         return false;
>>>        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
>>>         break;
>>
>> A comment would be nice here.  Like
>>
>>    /* For conversions between float and smaller integer types try whether we can
>>       use intermediate signed integer types to support the conversion.  */
>>
>>> +      if ((code == FLOAT_EXPR
>>> +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
>>> +         || (code == FIX_TRUNC_EXPR
>>> +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
>
> Is the FIX_TRUNC_EXPR case safe without some flag?
>
> #include <stdint.h>
> int32_t x = (int32_t)0x1.0p32;
> int32_t y = (int32_t)(int64_t)0x1.0p32;
>
> sets x to 2147483647 and y to 0.

Also, I think multi_step_cvt should influence the costs, since at
the moment we cost one statement but generate two.  This makes a
difference for SVE with VECT_COMPARE_COSTS.  Would changing it to:

	  vect_model_simple_cost (vinfo, stmt_info,
				  ncopies * (multi_step_cvt + 1),
				  dt, ndts, slp_node,
				  cost_vec);

be OK?

There again, I wonder if we should handle this using patterns instead.
That makes both conversions explicit and therefore easier to cost.

E.g. for SVE, an integer extension is free if the source is a load,
and we do try to model that.  But it's difficult to handle if the
conversion is only implicit.

Thanks,
Richard

  reply	other threads:[~2023-06-21  9:32 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-02  1:00 liuhongt
2023-06-20  8:38 ` Richard Biener
2023-06-20  9:09   ` Hongtao Liu
2023-06-20  9:22     ` Richard Biener
2023-06-20 16:11       ` liuhongt
2023-06-21  7:49         ` Uros Bizjak
2023-06-21  8:19           ` Richard Biener
2023-06-21 14:37             ` Uros Bizjak
2023-06-22 20:36         ` Thiago Jung Bauermann
2023-06-21  9:10   ` Richard Sandiford
2023-06-21  9:32     ` Richard Sandiford [this message]
2023-06-21 11:04       ` Richard Biener
2023-06-21 11:20         ` Richard Sandiford
2023-06-21 20:39         ` Joseph Myers
2023-06-22  7:32           ` Richard Biener
2023-07-12  9:19             ` Robin Dapp

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=mptttv188k3.fsf@arm.com \
    --to=richard.sandiford@arm.com \
    --cc=crazylht@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hjl.tools@gmail.com \
    --cc=hongtao.liu@intel.com \
    --cc=richard.guenther@gmail.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).