This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.


On Fri, Feb 13, 2015 at 5:06 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> 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

Thanks for reviewing. Hale wants me to continue his work because he
will be in holiday in next ten days. The check of asm is added. Is
this one OK?

BR,
Terry

Attachment: pr64818-combine-user-specified-register.patch-3
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]