This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- Cc: Hale Wang <hale dot wang at arm dot com>, "'GCC Patches'" <gcc-patches at gcc dot gnu dot org>, richard dot sandiford at arm dot com
- Date: Fri, 13 Feb 2015 09:06:58 +0000
- Subject: Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
- Authentication-results: sourceware.org; auth=none
- References: <002701d04409$a20efcb0$e62cf610$ at arm dot com> <87pp9fcehe dot fsf at e105548-lin dot cambridge dot arm dot com> <20150212221552 dot GA25883 at gate dot crashing dot org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Feb 12, 2015 at 03:54:21PM +0000, Richard Sandiford wrote:
>> "Hale Wang" <hale.wang@arm.com> writes:
>> > Ping?
>
> It's not a regression (or is it?), so it is not appropriate for stage4.
>
>
>> >> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644
>> >> --- a/gcc/combine.c
>> >> +++ b/gcc/combine.c
>> >> @@ -1904,6 +1904,12 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3,
>> >> rtx_insn *pred ATTRIBUTE_UNUSED,
>> >> set = expand_field_assignment (set);
>> >> src = SET_SRC (set), dest = SET_DEST (set);
>> >>
>> >> + /* Don't combine if dest contains a user specified register, because
>> > the
>> >> + user specified register (same with dest) in i3 would be replaced by
>> > the
>> >> + src of insn which might be different with the user's expectation.
>> >> + */ if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
>> >> (dest))
>> >> + return 0;
>>
>> I suppose this is similar to Andrew's comment, but I think the rule
>> is that it's invalid to replace a REG_USERVAR_P operand in an inline asm.
>
> Why not? You probably mean register asm, not all user variables?
Yeah, meant hard REG_USERVAR_P, sorry, as for the patch.
>> Outside of an inline asm we make no guarantee about whether something is
>> stored in a particular register or not.
>>
>> So IMO we should be checking whether either INSN or I3 is an asm as well
>> as the above.
>
> [ INSN can never be an asm, that is already refused by can_combine_p. ]
>
> We do not guarantee things will end up in the specified reg (except for asm),
> but will it hurt to leave things in the reg the user said it should be in, even
> if we do not guarantee this behaviour?
Whether it does not, making the test unnecessarily wide is at best only
going to paper over problems elsewhere. I really think we should test
for i3 being an asm.
Thanks,
Richard