Re: [PATCH][RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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.

Ok, thanks for the explanation.
When investigating this bug I tried a patch identical to yours and it had worked just fine.
My patch was just an alternative approach to the same issue.
I'll retest it just to double-check and I'll incorporate the testsuite additions.

Thanks for your help!

Kyrill


> Bernd