Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
Terry Guo writes:
> On Thu, Feb 26, 2015 at 1:55 PM, Segher Boessenkool
> wrote:
>> On Tue, Feb 17, 2015 at 11:39:34AM +0800, Terry Guo wrote:
>>> On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool
>>> wrote:
>>> > Hi Terry,
>>> >
>>> > I still think this is stage1 material.
>>> >
>>> >> + /* Don't combine if dest contains a user specified register and
>>> >> i3 contains
>>> >> + ASM_OPERANDS, 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. */
>>> >
>>> > "Do not eliminate a register asm in an asm input" or similar? Text
>>> > explaining why REG_USERVAR_P && HARD_REGISTER_P works here would be
>>> > good to have, too.
>>
>>> diff --git a/gcc/combine.c b/gcc/combine.c
>>> index f779117..aeb2854 100644
>>> --- a/gcc/combine.c
>>> +++ b/gcc/combine.c
>>> @@ -1779,7 +1779,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn
>>> *pred ATTRIBUTE_UNUSED,
>>> {
>>>int i;
>>>const_rtx set = 0;
>>> - rtx src, dest;
>>> + rtx src, dest, asm_op;
>>>rtx_insn *p;
>>> #ifdef AUTO_INC_DEC
>>>rtx link;
>>> @@ -1914,6 +1914,14 @@ 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);
>>>
>>> + /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether DEST is a user
>>> + specified register, and do not eliminate such register if it is in an
>>> + asm input because we may end up with something different with user's
>>> + expectation. */
>>
>> That doesn't explain why this will hit (almost) only on register asms.
>> The user's expectation doesn't matter that much either: GCC would violate
>> its own documentation / promises, that matters more ;-)
>>
>>> + if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
>>> + && ((asm_op = extract_asm_operands (PATTERN (i3))) != NULL))
>>
>> You do not need the temporary variable, nor the != 0 or the extra parens;
>> just write
>>
>> && extract_asm_operands (PATTERN (i3))
>>
>> Cheers,
>>
>>
>> Segher
>
> Thanks for comments. Patch is updated now. Please review again.
Looks good to me FWIW.
Thanks,
Richard
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Thu, Feb 26, 2015 at 1:55 PM, Segher Boessenkool
wrote:
> On Tue, Feb 17, 2015 at 11:39:34AM +0800, Terry Guo wrote:
>> On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool
>> wrote:
>> > Hi Terry,
>> >
>> > I still think this is stage1 material.
>> >
>> >> + /* Don't combine if dest contains a user specified register and i3
>> >> contains
>> >> + ASM_OPERANDS, 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. */
>> >
>> > "Do not eliminate a register asm in an asm input" or similar? Text
>> > explaining why REG_USERVAR_P && HARD_REGISTER_P works here would be
>> > good to have, too.
>
>> diff --git a/gcc/combine.c b/gcc/combine.c
>> index f779117..aeb2854 100644
>> --- a/gcc/combine.c
>> +++ b/gcc/combine.c
>> @@ -1779,7 +1779,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn
>> *pred ATTRIBUTE_UNUSED,
>> {
>>int i;
>>const_rtx set = 0;
>> - rtx src, dest;
>> + rtx src, dest, asm_op;
>>rtx_insn *p;
>> #ifdef AUTO_INC_DEC
>>rtx link;
>> @@ -1914,6 +1914,14 @@ 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);
>>
>> + /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether DEST is a user
>> + specified register, and do not eliminate such register if it is in an
>> + asm input because we may end up with something different with user's
>> + expectation. */
>
> That doesn't explain why this will hit (almost) only on register asms.
> The user's expectation doesn't matter that much either: GCC would violate
> its own documentation / promises, that matters more ;-)
>
>> + if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
>> + && ((asm_op = extract_asm_operands (PATTERN (i3))) != NULL))
>
> You do not need the temporary variable, nor the != 0 or the extra parens;
> just write
>
> && extract_asm_operands (PATTERN (i3))
>
> Cheers,
>
>
> Segher
Thanks for comments. Patch is updated now. Please review again.
BR,
Terry
pr64818-combine-user-specified-register.patch-5
Description: Binary data
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Tue, Feb 17, 2015 at 11:39:34AM +0800, Terry Guo wrote:
> On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool
> wrote:
> > Hi Terry,
> >
> > I still think this is stage1 material.
> >
> >> + /* Don't combine if dest contains a user specified register and i3
> >> contains
> >> + ASM_OPERANDS, 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. */
> >
> > "Do not eliminate a register asm in an asm input" or similar? Text
> > explaining why REG_USERVAR_P && HARD_REGISTER_P works here would be
> > good to have, too.
> diff --git a/gcc/combine.c b/gcc/combine.c
> index f779117..aeb2854 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -1779,7 +1779,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn
> *pred ATTRIBUTE_UNUSED,
> {
>int i;
>const_rtx set = 0;
> - rtx src, dest;
> + rtx src, dest, asm_op;
>rtx_insn *p;
> #ifdef AUTO_INC_DEC
>rtx link;
> @@ -1914,6 +1914,14 @@ 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);
>
> + /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether DEST is a user
> + specified register, and do not eliminate such register if it is in an
> + asm input because we may end up with something different with user's
> + expectation. */
That doesn't explain why this will hit (almost) only on register asms.
The user's expectation doesn't matter that much either: GCC would violate
its own documentation / promises, that matters more ;-)
> + if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
> + && ((asm_op = extract_asm_operands (PATTERN (i3))) != NULL))
You do not need the temporary variable, nor the != 0 or the extra parens;
just write
&& extract_asm_operands (PATTERN (i3))
Cheers,
Segher
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool wrote: > Hi Terry, > > I still think this is stage1 material. > >> + /* Don't combine if dest contains a user specified register and i3 >> contains >> + ASM_OPERANDS, 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. */ > > "Do not eliminate a register asm in an asm input" or similar? Text > explaining why REG_USERVAR_P && HARD_REGISTER_P works here would be > good to have, too. > >> + if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest) >> + && (GET_CODE (PATTERN (i3)) == SET >> + && GET_CODE (SET_SRC (PATTERN (i3))) == ASM_OPERANDS)) >> +return 0; > > That works only for asms with exactly one output. You want > extract_asm_operands. > > > Segher Thanks Segher. Patch is updated per you suggestion. Is this one ok for stage 1? BR, Terry pr64818-combine-user-specified-register.patch-4 Description: Binary data
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
Hi Terry, I still think this is stage1 material. > + /* Don't combine if dest contains a user specified register and i3 contains > + ASM_OPERANDS, 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. */ "Do not eliminate a register asm in an asm input" or similar? Text explaining why REG_USERVAR_P && HARD_REGISTER_P works here would be good to have, too. > + if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest) > + && (GET_CODE (PATTERN (i3)) == SET > + && GET_CODE (SET_SRC (PATTERN (i3))) == ASM_OPERANDS)) > +return 0; That works only for asms with exactly one output. You want extract_asm_operands. Segher
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 wrote: > Segher Boessenkool writes: >> On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: >>> "Hale Wang" 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 pr64818-combine-user-specified-register.patch-3 Description: Binary data
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
Segher Boessenkool writes: > On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: >> "Hale Wang" 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
RE: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
> -Original Message- > From: Segher Boessenkool [mailto:seg...@kernel.crashing.org] > Sent: Friday, February 13, 2015 6:16 AM > To: Hale Wang; 'GCC Patches'; Richard Sandiford > Subject: Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns > if a volatile register is contained. > > On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: > > "Hale Wang" 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? > > > 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. ] > Indeed. If INSN is an asm operand, it's already refused by can_combine_p. Hale. > 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? > > > Segher
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On 02/12/15 15:15, Segher Boessenkool wrote: On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: "Hale Wang" writes: Ping? It's not a regression (or is it?), so it is not appropriate for stage4. That's the big question, of course. [ 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? I doubt it could hurt except in extreme corner cases where the value gets used later in some insn where the user register was inappropriate. That will cause a spill to copy from the user register to a scratch of the appropriate type. That's, of course, if I'm reading this correctly... jeff
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: > "Hale Wang" 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? > 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? Segher
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
"Hale Wang" writes: > Ping? > >> -Original Message- >> From: Hale Wang [mailto:hale.w...@arm.com] >> Sent: Thursday, January 29, 2015 9:58 AM >> To: Hale Wang; 'Segher Boessenkool' >> Cc: GCC Patches >> Subject: RE: [PATCH] [gcc, combine] PR46164: Don't combine the insns if a >> volatile register is contained. >> >> Hi Segher, >> >> I have updated the patch as you suggested. Both the patch and the >> changelog are attached. >> >> By the way, the test case provided by Tim Pambor in PR46164 was a > different >> bug with PR46164. So I resubmitted the bug in >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64818. >> And this patch is just used to fix this bug. Is it OK for you? >> >> Thanks, >> Hale >> >> gcc/ChangeLog: >> 2015-01-27 Segher Boessenkool >> Hale Wang >> >> PR rtl-optimization/64818 >> * combine.c (can_combine_p): Don't combine the insn if >> the dest of insn is a user specified register. >> >> gcc/testsuit/ChangeLog: >> 2015-01-27 Segher Boessenkool >> Hale Wang >> >> PR rtl-optimization/64818 >> * gcc.target/arm/pr64818.c: New test. >> >> >> 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. 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. Thanks, Richard
