Re: [PATCH] Optional alternative base_expr in finding basis for CAND_REFs

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Nov 14, 2013 at 12:25 AM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote:
> Hi Bill,
>
>
> On 11/13/13 20:54, Bill Schmidt wrote:
>>
>> Hi Yufeng,
>>
>> The second version of your original patch is ok with me with the
>> following changes.  Sorry for the little side adventure into the
>> next-interp logic; in the end that's going to hurt more than it helps in
>> this case.  Thanks for having a look at it, anyway.  Thanks also for
>> cleaning up this version to be less intrusive to common interfaces; I
>> appreciate it.
>
>
> Thanks a lot for the review.  I've attached an updated patch with the
> suggested changes incorporated.
>
> For the next-interp adventure, I was quite happy to do the experiment; it's
> a good chance of gaining insight into the pass.  Many thanks for your prompt
> replies and patience in guiding!
>
>
>> Everything else looks OK to me.  Please ask Richard for final approval,
>> as I'm not a maintainer.
>
>
> Hi Richard, would you be happy to OK the patch?

Hmm,

+static tree
+get_alternative_base (tree base)
+{
+  tree *result = (tree *) pointer_map_contains (alt_base_map, base);
+
+  if (result == NULL)
+    {
+      tree expr;
+      aff_tree aff;
+
+      tree_to_aff_combination_expand (base, TREE_TYPE (base),
+                                     &aff, &name_expansions);
+      aff.offset = tree_to_double_int (integer_zero_node);
+      expr = aff_combination_to_tree (&aff);
+
+      result = (tree *) pointer_map_insert (alt_base_map, base);
+      gcc_assert (!*result);

I believe this cache will never hit (unless you repeatedly ask for
the exact same statement?) - any non-trivial 'base' trees are
not shared and thus not pointer equivalent.

Also using tree_to_aff_combination_expand to get at - what
exactly? The address with any constant offset stripped?
Where do you re-construct that offset?  That is, aff.offset,
which you definitely need to get at a candidate?

+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-slsr" } */
+
+typedef int arr_2[50][50];
+
+void foo (arr_2 a2, int v1)
+{
+  int i, j;
+
+  i = v1 + 5;
+  j = i;
+  a2 [i-10] [j] = 2;
+  a2 [i] [j++] = i;
+  a2 [i+20] [j++] = i;
+  a2 [i-3] [i-1] += 1;
+  return;
+}
+
+/* { dg-final { scan-tree-dump-times "MEM" 5 "slsr" } } */
+/* { dg-final { cleanup-tree-dump "slsr" } } */

scanning for 5 MEMs looks non-sensical.  What transform do
you expect?  I see other slsr testcases do similar non-sensical
checking which is bad, too.

Richard.

> Regards,
>
> Yufeng
>
> gcc/
>
>         * gimple-ssa-strength-reduction.c: Include tree-affine.h.
>         (name_expansions): New static variable.
>         (alt_base_map): Ditto.
>         (get_alternative_base): New function.
>         (find_basis_for_candidate): For CAND_REF, optionally call
>         find_basis_for_base_expr with the returned value from
>         get_alternative_base.
>         (record_potential_basis): Add new parameter 'base' of type 'tree';
>         add an assertion of non-NULL base; use base to set node->base_expr.
>
>         (alloc_cand_and_find_basis): Update; call record_potential_basis
>         for CAND_REF with the returned value from get_alternative_base.
>         (execute_strength_reduction): Call pointer_map_create for
>         alt_base_map; call free_affine_expand_cache with &name_expansions.
>
> gcc/testsuite/
>
>         * gcc.dg/tree-ssa/slsr-41.c: New test.