Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67749: Do not emit separate SET insn in IF-ELSE case

[Jeff Law <law@redhat.com>]

On 11/03/2015 02:09 AM, Kyrill Tkachov wrote:
> Hi Jeff,
>
> On 02/11/15 22:46, Jeff Law wrote:
>> On 10/27/2015 08:49 AM, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> This patch fixes the gcc.dg/ifcvt-2.c test for x86_64 where we were
>>> failing to if-convert. This was because in my patch at
>>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228194 which
>>> tried to emit a SET to move the source of insn_a or insn_b (that came
>>> from the test block) into a temporary. A SET however, is not always
>>> enough. For example, on x86_64 in order for the resulting insn to be
>>> recognised it frequently needs to be in PARALLEL with a (clobber
>>> (reg:CC FLAGS_REG)). This leads to the insn not being recognised.
>> I don't think it affects the approach you've chosen, but I'm
>> mentioning it for future reference.
>>
>> gcse.c has some helper code (that probably ought to move into a more
>> generic file) that will test for this situation.  Search for the
>> instances of recog.  It essentially does something like
>>
>> emit_insn (gen_rtx_SET (...))
>>
>> And tries to recognize the result to determine if it's valid.
>
> you mean compute_can_copy and can_copy_p? I was not aware of those.
> Interesting, they look like they
> can be useful in places indeed. I'll keep them in mind for any future work.
Yes.  Those.


>>
>> What happens in the case were noce_emit_bb returns a failure? We've
>> modified the original insns to use the new pseudos.  Won't this result
>> in the original pseudo's uses using undefined values?
>
> We should be fine because we don't modify the original insns. We create
> a copy of them i.e.
> rtx_insn *copy_of_a = as_a <rtx_insn *> (copy_rtx (insn_a));
>
> and modify the SET_DEST of that. The original insn should still remain
> intact if any step in
> noce_try_cmove_arith fails, so we can revert back to the original sequence.
In that case, OK for the trunk.

Thanks,
jeff