On Tue, Apr 10, 2018 at 2:52 AM, Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> 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?

              if (temp
                  /* Make sure we are not in a loop latch block.  */
                  || gimple_bb (stmt) == gimple_bb (use_stmt)
-                 || dominated_by_p (CDI_DOMINATORS,
-                                    gimple_bb (stmt), gimple_bb (use_stmt))
                  /* We can look through PHIs to regions post-dominating

you are removing part of the latch-block check but not the other.

+                 /* If stmt dominates PHI stmt, follow the PHI stmt.  */
+                 if (!temp)

well, just do this check earlier.  Or rather, it's already done in the
test above.

+                     /* Makesure the use stmt found is post dominated.  */
+                     && dominated_by_p (CDI_POST_DOMINATORS,
+                                        gimple_bb (stmt_outer),
gimple_bb (inner_use_stmt))

I think that this check also covers gimple_bb (stmt_outer) ==
gimple_bb (inner_use_stmt)
so for that case you'd need to check stmt dominance.  But better just give up?

Given the simple testcases you add I wonder if you want a cheaper
implementation,
namely check that when reaching a loop PHI the only aliasing stmt in
its use-chain
is the use_stmt you reached the PHI from.  That would avoid this and the tests
for the store being redundant and simplify the patch considerably.

Thanks,
Richard.

> 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.

Reply via email to