From: "Li, Pan2" <pan2.li@intel.com>
To: Richard Sandiford <richard.sandiford@arm.com>,
Jeff Law <jeffreyalaw@gmail.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
"juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai>,
"Wang, Yanzhang" <yanzhang.wang@intel.com>,
"kito.cheng@gmail.com" <kito.cheng@gmail.com>,
"richard.guenther@gmail.com" <richard.guenther@gmail.com>
Subject: RE: [PATCH v2] DSE: Allow vector type for get_stored_val when read < store
Date: Sun, 12 Nov 2023 02:30:32 +0000 [thread overview]
Message-ID: <MW5PR11MB59080154F63CD29F85E7D759A9ACA@MW5PR11MB5908.namprd11.prod.outlook.com> (raw)
In-Reply-To: <mptsf5c2u1b.fsf@arm.com>
Thanks Richard S and Jeff for comments.
> Did you want to use known_le so that you'd pick up the case when the two
> modes are the same size? Or was known_lt the test you really wanted
> (and if so, why).
Take known_lt in v2 due to consideration that leave the equal go to original code path.
Just have a try for known_le and got sorts of ICE when test, I bet it may be related to the
latent bug as Richard S mentioned.
> instead. Alternatively, we could remove the is_constant condition
> and fix PR87815 in a different way, e.g. by protecting the
> smallest_int_mode_for_size with a tighter condition. That might
> allow a similar DSE optimisation to this patch for nonzero offsets,
> thanks to:
Thus, looks like we should fix the PR87815 from the way suggested by Richard S, before
we take known_le for vector here.
I will have a try soon and keep you posted.
Pan
-----Original Message-----
From: Richard Sandiford <richard.sandiford@arm.com>
Sent: Saturday, November 11, 2023 11:23 PM
To: Jeff Law <jeffreyalaw@gmail.com>
Cc: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; richard.guenther@gmail.com
Subject: Re: [PATCH v2] DSE: Allow vector type for get_stored_val when read < store
Jeff Law <jeffreyalaw@gmail.com> writes:
> On 11/8/23 23:08, pan2.li@intel.com wrote:
>> From: Pan Li <pan2.li@intel.com>
>>
>> Update in v2:
>> * Move vector type support to get_stored_val.
>>
>> Original log:
>>
>> This patch would like to allow the vector mode in the
>> get_stored_val in the DSE. It is valid for the read
>> rtx if and only if the read bitsize is less than the
>> stored bitsize.
>>
>> Given below example code with
>> --param=riscv-autovec-preference=fixed-vlmax.
>>
>> vuint8m1_t test () {
>> uint8_t arr[32] = {
>> 1, 2, 7, 1, 3, 4, 5, 3, 1, 0, 1, 2, 4, 4, 9, 9,
>> 1, 2, 7, 1, 3, 4, 5, 3, 1, 0, 1, 2, 4, 4, 9, 9,
>> };
>>
>> return __riscv_vle8_v_u8m1(arr, 32);
>> }
>>
>> Before this patch:
>> test:
>> lui a5,%hi(.LANCHOR0)
>> addi sp,sp,-32
>> addi a5,a5,%lo(.LANCHOR0)
>> li a3,32
>> vl2re64.v v2,0(a5)
>> vsetvli zero,a3,e8,m1,ta,ma
>> vs2r.v v2,0(sp) <== Unnecessary store to stack
>> vle8.v v1,0(sp) <== Ditto
>> vs1r.v v1,0(a0)
>> addi sp,sp,32
>> jr ra
>>
>> After this patch:
>> test:
>> lui a5,%hi(.LANCHOR0)
>> addi a5,a5,%lo(.LANCHOR0)
>> li a4,32
>> addi sp,sp,-32
>> vsetvli zero,a4,e8,m1,ta,ma
>> vle8.v v1,0(a5)
>> vs1r.v v1,0(a0)
>> addi sp,sp,32
>> jr ra
>>
>> Below tests are passed within this patch:
>>
>> * The x86 bootstrap and regression test.
>> * The aarch64 regression test.
>> * The risc-v regression test.
>>
>> PR target/111720
>>
>> gcc/ChangeLog:
>>
>> * dse.cc (get_stored_val): Allow vector mode if the read
>> bitsize is less than stored bitsize.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/riscv/rvv/base/pr111720-0.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-1.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-10.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-2.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-3.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-4.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-5.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-6.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-7.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-8.c: New test.
>> * gcc.target/riscv/rvv/base/pr111720-9.c: New test.
> We're always getting the lowpart here AFAICT and it appears that all the
> right thing should happen if gen_lowpart_common fails (it returns NULL,
> which bubbles up and is the right return value from get_stored_val if it
> can't be optimized).
Yeah, we should always be operating on the lowpart, but it looks
like there's a latent bug. This check:
if (gap.is_constant () && maybe_ne (gap, 0))
{
...
}
else ...
means that we ignore the gap if it's a nonzero runtime value.
I guess it should be:
if (maybe_ne (gap, 0))
{
if (!gap.is_constant ())
return NULL_RTX;
...
}
instead. Alternatively, we could remove the is_constant condition
and fix PR87815 in a different way, e.g. by protecting the
smallest_int_mode_for_size with a tighter condition. That might
allow a similar DSE optimisation to this patch for nonzero offsets,
thanks to:
if (multiple_p (shift, GET_MODE_BITSIZE (new_mode))
&& known_le (GET_MODE_SIZE (new_mode), GET_MODE_SIZE (store_mode)))
{
/* Try to implement the shift using a subreg. */
...
> Did you want to use known_le so that you'd pick up the case when the two
> modes are the same size? Or was known_lt the test you really wanted
> (and if so, why).
Agree it should be known_le FWIW.
Thanks,
Richard
next prev parent reply other threads:[~2023-11-12 2:30 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-02 3:14 [PATCH v1] EXPMED: Allow vector mode for DSE extract_low_bits [PR111720] pan2.li
2023-11-02 8:19 ` Richard Biener
2023-11-02 12:17 ` Li, Pan2
2023-11-09 6:08 ` [PATCH v2] DSE: Allow vector type for get_stored_val when read < store pan2.li
2023-11-09 16:16 ` Jeff Law
2023-11-11 15:23 ` Richard Sandiford
2023-11-12 2:30 ` Li, Pan2 [this message]
2023-11-13 3:25 ` Li, Pan2
2023-11-12 12:27 ` [PATCH v3] " pan2.li
2023-11-13 3:22 ` [PATCH v4] " pan2.li
2023-11-13 20:11 ` Jeff Law
2023-11-15 0:18 ` Li, Pan2
2023-11-15 0:24 ` Li, Pan2
2023-11-22 2:30 ` Li, Pan2
2023-11-22 8:02 ` Richard Biener
2023-11-22 11:38 ` Li, Pan2
2023-11-22 18:39 ` Richard Sandiford
2023-11-23 1:20 ` Li, Pan2
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=MW5PR11MB59080154F63CD29F85E7D759A9ACA@MW5PR11MB5908.namprd11.prod.outlook.com \
--to=pan2.li@intel.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
--cc=juzhe.zhong@rivai.ai \
--cc=kito.cheng@gmail.com \
--cc=richard.guenther@gmail.com \
--cc=richard.sandiford@arm.com \
--cc=yanzhang.wang@intel.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).