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

[Yufeng Zhang <Yufeng dot Zhang at arm dot com>]

Ping~

http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03360.html

Thanks,
Yufeng

On 11/26/13 15:02, Yufeng Zhang wrote:
> On 11/26/13 12:45, Richard Biener wrote:
> > On Thu, Nov 14, 2013 at 12:25 AM, Yufeng Zhang<Yufeng.Zhang@arm.com>   wrote:
> > > On 11/13/13 20:54, Bill Schmidt wrote:
> > > > 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.
>
> Yes, you are right about the non-trivial 'base' tree are rarely shared.
>    The cached is introduced mainly because get_alternative_base () may be
> called twice on the same 'base' tree, once in the
> find_basis_for_candidate () for look-up and the other time in
> alloc_cand_and_find_basis () for record_potential_basis ().  I'm happy
> to leave out the cache if you think the benefit is trivial.
>
> > 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?
>
> As explained in the previous RFC emails, the expanded and
> constant-offset-stripped base expr is only used for the purpose of basis
> look-up.  The corresponding candidate still has the unexpanded base expr
> as its 'base_expr', therefore the info on the constant offset is not
> lost and doesn't need to be re-constructed.
>
> > +/* { 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.
>
> As the slsr optimizes CAND_REF candidates by simply lowering them to
> MEM_REF from e.g. ARRAY_REF, I think scanning for the number of MEM_REFs
> is an effective check.  Alternatively, I can add a follow-up patch to
> add some dumping facility in replace_ref () to print out the replacing
> actions when -fdump-tree-slsr-details is on.
>
> I hope these can address your concerns.
>
>
> Regards,
> Yufeng
>
>
>
> > 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.

Attachment: patch-use-affine-v3.txt
Description: Text document