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 17/11/15 23:11, Bernd Schmidt wrote:
> On 11/17/2015 02:03 PM, Kyrill Tkachov wrote:
> > +      || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn))))
> >      return false;
>
> > Well, I think the statement we want to make is
> > "return false from this function if the two expressions contain the same
> > register number".
>
> I looked at it again and I think I'll need more time to really figure out what's going on in this pass.
>
> However, about the above... either I'm very confused, or the actual statement here is "return false _unless_ the two expressions contain the same register number". In the testcase, the regs in question are ax and bx, which is then 
> rejected if the patch has been applied.

I'm sorry, it is my mistake in explaining. I meant to say:
"return false from this function unless the two expressions contain the same
 register number"

> (gdb) p tmp_reg
> (reg:SI 0 ax)
> (gdb) p cand->insn
> (insn 59 117 60 7 (set (reg:SI 4 si [orig:107 D.1813 ] [107])
>         (sign_extend:SI (reg:HI 3 bx [orig:99 D.1813 ] [99])))
>
> And I think it would be better to strengthen that to "return false unless registers are identical". (From the above it's clear however that a plain rtx_equal_p wouldn't work since there's an extension in the operand).

So the three relevant instructions are:
I1: (set (reg:HI 0 ax)
     (mem:HI (symbol_ref:DI ("f"))))

I2: (set (reg:SI 3 bx)
     (if_then_else:SI (eq (reg:CCZ 17 flags)
                (const_int 0))
            (reg:SI 0 ax)
            (reg:SI 3 bx)))

I3: (set (reg:SI 4 si)
     (sign_extend:SI (reg:HI 3 bx)))

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.
Hope this helps.

> Also, I had another look at the testcase. It uses __builtin_printf and dg-output, which is at least unusual. Try to use the normal "if (cond) abort ()".

I did not try to remove the __builtin_printf because the use of 'f' there is needed to trigger this.
There are at least two other dups of this PR: 68328 and 68185 the testcases for which have an "if (cond) abort ();" form.
I'll use them instead.


> Bernd