On Fri, Jan 24, 2014 at 6:47 PM, Bingfeng Mei <b...@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
