On 04/09/2018 06:52 PM, Kugan Vivekanandarajah wrote:
> I would like to queue this patch for stage1 review.
>
> In DSE, while in dse_classify_store, as soon as we see a PHI use
> statement that is part of the loop, we are immediately giving up.
>
> As far as I understand, this can be improved. Attached patch is trying
> to walk the uses of the PHI statement (by recursively calling
> dse_classify_store) and then making sure the obtained store is indeed
> redundant.
>
> This is partly as reported in one of the testcase from PR44612. But
> this PR is about other issues that is not handled in this patch.
>
> Bootstrapped and regression tested on aarch64-linux-gnu with no new
> regressions.
>
> Is this OK for next stage1?
>
> Thanks,
> Kugan
>
>
> gcc/ChangeLog:
>
> 2018-04-10 Kugan Vivekanandarajah <kug...@linaro.org>
>
> * tree-ssa-dse.c (dse_classify_store): Handle recursive PHI.
> (dse_dom_walker::dse_optimize_stmt): Update call dse_classify_store.
>
> gcc/testsuite/ChangeLog:
>
> 2018-04-10 Kugan Vivekanandarajah <kug...@linaro.org>
>
> * gcc.dg/tree-ssa/ssa-dse-31.c: New test.
> * gcc.dg/tree-ssa/ssa-dse-32.c: New test.
>
>
> 0001-improve-dse.patch
>
>
> From 5751eaff3d1c263e8631d5a07e43fecaaa0e9d26 Mon Sep 17 00:00:00 2001
> From: Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org>
> Date: Tue, 10 Apr 2018 09:49:10 +1000
> Subject: [PATCH] improve dse
>
> ---
> gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-31.c | 16 ++++++++++
> gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-32.c | 23 ++++++++++++++
> gcc/tree-ssa-dse.c | 51
> ++++++++++++++++++++++++------
> 3 files changed, 81 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-31.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-32.c
>
> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> index 9220fea..3513fda 100644
> --- a/gcc/tree-ssa-dse.c
> +++ b/gcc/tree-ssa-dse.c
> @@ -521,11 +521,11 @@ live_bytes_read (ao_ref use_ref, ao_ref *ref, sbitmap
> live)
> Return TRUE if the above conditions are met, otherwise FALSE. */
>
> static dse_store_status
> -dse_classify_store (ao_ref *ref, gimple *stmt, gimple **use_stmt,
> - bool byte_tracking_enabled, sbitmap live_bytes)
> +dse_classify_store (ao_ref *ref, gimple *stmt_outer, gimple *stmt,
> + gimple **use_stmt, bool byte_tracking_enabled,
> + sbitmap live_bytes, unsigned cnt = 0)
> {
> gimple *temp;
> - unsigned cnt = 0;
>
> *use_stmt = NULL;
>
> @@ -556,9 +556,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt, gimple
> **use_stmt,
> {
> cnt++;
>
> + if (use_stmt == stmt_outer)
> + continue;
So is this really safe? This seems to catch the case where the
recursive call stumbles onto the same statement we're already
processing. ie, we've followed a loop backedge.
ISTM that further analysis here is needed -- don't you have to make
sure that USE_STMT does not read from REF? It could be a memmove call
for example.
I'm also struggling a little bit to see much value in handling this
case. In the included testcases we've got a memset in a loop where the
args do not vary across the loop iterations and there are no reads from
the memory location within the loop. How realistic is that?
If you're looking to improve DSE, the cases in 63185, 64380 and 79958
may be interesting.
