Pick up more address lowering cases for ivopt and tree-affine.c

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.


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.

Richard.

> jeff
>
>

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:
>>
>> Unless there's a PR for this problem, I think this needs to wait.
>
> I agree.  Btw, please split the patch.
Yes, I will get back to this after entering stage 1 again :)

Hi Richard,
I talked with you about clean strip_offset_1 up after this series of
base simplification patches, but I realized it's not safe because
affine facility has it's limit, like can only support 8 elements.
Though the cleanup passes bootstrap and test on x86/x86_64 and most of
codes in strip_offset_1 won't be executed usually, I guess we'd better
to live with it, so what do you think?

Thanks,
bin

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

On Fri, Dec 6, 2013 at 11:40 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:
>>>
>>> Unless there's a PR for this problem, I think this needs to wait.
>>
>> I agree.  Btw, please split the patch.
> Yes, I will get back to this after entering stage 1 again :)
>
> Hi Richard,
> I talked with you about clean strip_offset_1 up after this series of
> base simplification patches, but I realized it's not safe because
> affine facility has it's limit, like can only support 8 elements.
> Though the cleanup passes bootstrap and test on x86/x86_64 and most of
> codes in strip_offset_1 won't be executed usually, I guess we'd better
> to live with it, so what do you think?

Not sure - I'm lacking some context here ;)  If you have a cleanup patch
fine - WRT the affine limit of 8 elements, further elements will just
add to the rest tree.  This is to limit compile-time.

Richard.

> Thanks,
> bin
>
>>
>> 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.
>>
>> Richard.
>>
>>> jeff
>>>
>>>
>
>
>
> --
> Best Regards.

On Fri, Dec 6, 2013 at 7:20 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Dec 6, 2013 at 11:40 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:
>>>>
>>>> Unless there's a PR for this problem, I think this needs to wait.
>>>
>>> I agree.  Btw, please split the patch.
>> Yes, I will get back to this after entering stage 1 again :)
>>
>> Hi Richard,
>> I talked with you about clean strip_offset_1 up after this series of
>> base simplification patches, but I realized it's not safe because
>> affine facility has it's limit, like can only support 8 elements.
>> Though the cleanup passes bootstrap and test on x86/x86_64 and most of
>> codes in strip_offset_1 won't be executed usually, I guess we'd better
>> to live with it, so what do you think?
>
> Not sure - I'm lacking some context here ;)  If you have a cleanup patch
> fine - WRT the affine limit of 8 elements, further elements will just
> add to the rest tree.  This is to limit compile-time.
Yes, so it's possible to have COMPONENT_REF stored in rest tree if we
reach the 8 elements limit.  In this case the COMPONENT_REF will be
visited by the code in strip_offset_1, although it sounds unlikely to
happen.

Thanks,
bin
>
> Richard.