[PATCH GCC]Pick up more address lowering cases for ivopt and tree-affine.c

[Richard Biener]

On Tue, May 6, 2014 at 10:39 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Fri, Dec 6, 2013 at 6:19 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Mon, Nov 25, 2013 at 7:41 PM, Jeff Law <law@redhat.com> wrote:
>>> On 11/25/13 02:22, bin.cheng wrote:
>>>>
>>>> Hi,
>>>> I previously committed two patches lowering complex address expression for
>>>> IVOPT at http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00546.html and
>>>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01103.html
>>>> When I bootstrapping GCC I found there were some peculiar cases like
>>>> &MEM[ptr+CST] + xxxx, which should be handled too.  This patch consists
>>>> below two changes:
>>>>
>>>> 1) change in alloc_iv:
>>>> Original code lowers top level complex address expressions like
>>>> &MEM[ptr+off].  The patch relaxes check condition in order to lower
>>>> expressions like &MEM[ptr+off] + xxx, just as the BASE from below dump:
>>>> use 2
>>>>    generic
>>>>    in statement _595 = &MEM[(void *)&this_prg + 36B] + _594;
>>>>
>>>>    at position
>>>>    type struct gcov_bucket_type *
>>>>    base (struct gcov_bucket_type *) &MEM[(void *)&this_prg + 36B] +
>>>> (sizetype) ((unsigned int) (src_i_683 + -1) * 20)
>>>>    step 4294967276
>>>>    base object (void *) &this_prg
>>>>    related candidates
>>>>
>>>> 2) change in tree_to_aff_combination:
>>>> The function get_inner_reference returns "&MEM[ptr+off]" as the core for
>>>> input like the memory ADDRESS in below dump:
>>>> use 2
>>>>    address
>>>>    in statement _59 = MEM[(const struct gcov_ctr_summary *)summary_22(D) +
>>>> 4B].histogram[h_ix_111].min_value;
>>>>
>>>>    at position MEM[(const struct gcov_ctr_summary *)summary_22(D) +
>>>> 4B].histogram[h_ix_111].min_value
>>>>    type const gcov_type *
>>>>    base (const gcov_type *) &MEM[(const struct gcov_ctr_summary
>>>> *)summary_22(D) + 4B] + 36
>>>>    step 20
>>>>    base object (void *) summary_22(D)
>>>>    related candidates
>>>>
>>>> Which can be further reduced into something like "summary_22(D) + 40B".
>>>> This change is necessary for the first one, because I am using
>>>> tree_to_aff_combination rather than get_inner_reference_aff now.
>>>>
>>>> Bootstrap and test on x86/x86_64/arm.  Is it OK?
>>>>
>>>> Thanks.
>>>> bin
>>>>
>>>> 2013-11-25  Bin Cheng  <bin.cheng@arm.com>
>>>>
>>>>         * tree-ssa-loop-ivopts.c (contain_complex_addr_expr): New.
>>>>         (alloc_iv): Lower more cases by calling contain_complex_addr_expr
>>>>         and tree_to_aff_combination.
>>>>         * tree-affine.c (tree_to_aff_combination): Handle &MEM[ptr+CST]
>>>>         in core part of complex reference.
>>>>
>>>> gcc/testsuite/ChangeLog
>>>> 2013-11-25  Bin Cheng  <bin.cheng@arm.com>
>>>>
>>>>         * gcc.dg/tree-ssa/ivopts-lower_base.c: New test.
>>>
>>> Unless there's a PR for this problem, I think this needs to wait.
>>
>> I agree.  Btw, please split the patch.
>>
>> Index: gcc/tree-affine.c
>> ===================================================================
>> --- gcc/tree-affine.c    (revision 205087)
>> +++ gcc/tree-affine.c    (working copy)
>> @@ -328,7 +328,19 @@ tree_to_aff_combination (tree expr, tree type, aff
>>                   double_int::from_uhwi (bitpos / BITS_PER_UNIT));
>>        core = build_fold_addr_expr (core);
>>        if (TREE_CODE (core) == ADDR_EXPR)
>> -    aff_combination_add_elt (comb, core, double_int_one);
>> +    {
>> +      /* Handle &MEM[ptr + CST] in core part of complex reference.  */
>> +      if (TREE_CODE (TREE_OPERAND (core, 0)) == MEM_REF)
>> +        {
>> +          core = TREE_OPERAND (core, 0);
>> +          tree_to_aff_combination (TREE_OPERAND (core, 0), type, &tmp);
>> +          aff_combination_add (comb, &tmp);
>> +          tree_to_aff_combination (TREE_OPERAND (core, 1), sizetype, &tmp);
>> +          aff_combination_add (comb, &tmp);
>> +        }
>> +      else
>> +        aff_combination_add_elt (comb, core, double_int_one);
>> +    }
>>        else
>>      {
>>        tree_to_aff_combination (core, type, &tmp)
>>
>> please handle the offset before taking the address, thus
>>
>>   if (TREE_CODE (core) == MEM_REF)
>>     {
>>        add constant offset;
>>        core = TREE_OPERAND (core, 0);
>>     }
>>   else
>>     core = build_fold_addr_expr (core);
>>
>> that simplifies things and avoids the address building.
>>
> Hi,
> I split the patch into two and updated the test case.
> The patches pass bootstrap/tests on x86/x86_64, also pass test on arm cortex-m3.
> Is it OK?
>
> Thanks,
> bin
>
> PATCH 1:
>
> 2014-05-06  Bin Cheng  <bin.cheng@arm.com>
>
>     * gcc/tree-affine.c (tree_to_aff_combination): Handle MEM_REF for
>     core part of address expressions.

No gcc/ in the changelog

Simplify that by using aff_combination_add_cst:

+      if (TREE_CODE (core) == MEM_REF)
+       {
+         aff_combination_add_cst (comb, mem_ref_offset (core));
+         core = TREE_OPERAND (core, 0);

patch 1 is ok with that change.

> PATCH 2:
>
> 2014-05-06  Bin Cheng  <bin.cheng@arm.com>
>
>     * gcc/tree-ssa-loop-ivopts.c (contain_complex_addr_expr): New.
>     (alloc_iv): Lower base expressions containing ADDR_EXPR.

So this "lowers" addresses(?) which are based on &<not-decl>,
like &a[0] + 4 but not &a + 4.  I question this odd choice.  ISTR
when originally introducing address lowering (in rev. 204497) I was
concerned about the cost?

Now I wonder why we bother to convert the lowered form back
to trees to store it in ->base and not simply keep (and always
compute) the affine expansion only.  Thus, change struct iv
to have aff_tree base instead of tree base?

Can you see what it takes to do such change?  At the point
we replace uses we go into affine representation again anyway.

Thanks,
Richard.

> gcc/testsuite/ChangeLog
> 2014-05-06  Bin Cheng  <bin.cheng@arm.com>
>
>     * gcc.dg/tree-ssa/ivopts-lower_base.c: New test.
>
>
>
> --
> Best Regards.