Hi Bernd,

On 20/11/15 01:41, Bernd Schmidt wrote:
>>> I1 is def_insn, I3 is cand->insn. tmp_reg is 'ax'. What we want to do
>>> is reject this transformation
>>> because the destination of def_insn (aka I1), that is 'ax', is not the
>>> operand of the extend operation
>>> in cand->insn (aka I3). As you said, rtx_equal won't work on just
>>> SET_SRC (PATTERN (cand->insn)) because
>>> it's an extend operation. So reg_overlap_mentioned should be appropriate.
>
> Yeah, so just use the src_reg variable for the comparison. I still don't see why you wouldn't want to use the stronger test. But the whole thing still feels not completely ideal somehow, so after reading through ree.c for a while and 
> getting a better feeling for how it works, I think the following (which you said is equivalent) would be the most understandable and direct fix.
>
> You said that the two tests should be equivalent, and I agree. I've not found cases where the change makes a difference, other than the testcase. Would you mind running this version through the testsuite and committing if it passes?
>
> I've shrunk the comment; massive explanations like this for every bug are inappropriate IMO, and the example also duplicates an earlier comment in the same function. And, as I said earlier, the way you placed the comment is confusing 
> because only one part of the following if statement is related to it.
>

Thanks for the comments, here is the final patch that I'll be committing.
It passed testing on arm, aarch64, x86_64.

Thanks,
Kyrill


2015-11-23  Bernd Schmidt <bschmidt@redhat.com>
             Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68194
     PR rtl-optimization/68328
     PR rtl-optimization/68185
     * ree.c (combine_reaching_defs): Reject copy_needed case if
     copies_list is not empty.

2015-11-23  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68194
     * gcc.c-torture/execute/pr68185.c: Likewise.
     * gcc.c-torture/execute/pr68328.c: Likewise.


>
> Bernd