Re: Suspected bugs in ptr_difference_const & split_address_to_core_and_offset

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

On Fri, Jan 24, 2014 at 6:47 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> Hi,
> I experienced an issue in our port, which I suspected due to bugs
> in ptr_difference_const & split_address_to_core_and_offset. Basically,
> ptr_difference_const, called by ivopts pass, tries to evaluate
> whether e1 & e2 differ by a const. In this example,
>
> e1 is (addr_expr (mem_ref (ssa_name1, 8))
> e2 is just ssa_name1.
>
> It is obvious to me that ptr_difference_const should return true. But
> it calls split_address_to_core_and_offset to split e1 to some base pointer
> and offset. split_addess_to_core_and_offset in turn calls get_inner_reference
> to do it. get_inner_reference cannot handle (mem_ref (ssa_name1, 8)),
> it just returns the same expression back.
>
> get_inner_reference function
>         case MEM_REF:
>           /* Hand back the decl for MEM[&decl, off].  */
>           if (TREE_CODE (TREE_OPERAND (exp, 0)) == ADDR_EXPR)
>             {
>               tree off = TREE_OPERAND (exp, 1);
>               if (!integer_zerop (off))
>                 {
>                   double_int boff, coff = mem_ref_offset (exp);
>                   boff = coff.alshift (BITS_PER_UNIT == 8
>                                        ? 3 : exact_log2 (BITS_PER_UNIT),
>                                        HOST_BITS_PER_DOUBLE_INT);
>                   bit_offset += boff;
>                 }
>               exp = TREE_OPERAND (TREE_OPERAND (exp, 0), 0);
>             }
>           goto done;
>
> Then in ptr_difference_const, we get core1 as (mem_ref (ssa_name1, 8))
> and toffset1 is empty. ptr_difference_const will return false as result.

That's because get_inner_reference is the wrong tool to ask for
a base _address_ IMHO.  In theory get_inner_reference could
return MEM[ptr, 0] of course but that requires building a new tree
which isn't the suitable thing to do here.

What you want is a get_base_address_and_constant_offset_part.
It may be as simple as wrapping get_inner_reference
to perform the final step and adjust the kind of tree it is supposed to
return.

>
> There is another possible bug in ptr_difference_const. If one of toffset1
> & toffset2 is true, why it returns false?  The comment doesn't make sense
> to me. In this example, toffset1 should be 8 and toffset2 should be empty.

No, I think in this example bitpos should have the 8, not toffset.  toffset
are supposed to be non-constant parts.

I think the fix belongs into split_address_to_core_and_offset, handling
MEM[X, CST], avoiding the build_fold_addr_expr_loc and adjusting
pbitpos for CST.

Richard.

> No way it should return false.
>
>   else if (toffset1 || toffset2)
>     {
>      /* If only one of the offsets is non-constant, the difference cannot
>         be a constant.  */
>       return false;
>     }
>
>
> Any comment? I would like to submit a patch for it. The problem is I don't
> have an reproducible example on x86 or other public targets. I ran through
> the x86-64 tests and didn't hit a single case that meets this condition.
> e1 is (addr_expr (mem_ref (ssa_name1, 8))
> e2 is just ssa_name1.
> Not sure about other targets though.
>
> Thanks,
> Bingfeng