This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Ping^2 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
- From: "Hale Wang" <hale dot wang at arm dot com>
- To: "'Richard Sandiford'" <rdsandiford at googlemail dot com>, "Terry Guo" <flameroc at gmail dot com>
- Cc: "Segher Boessenkool" <segher at kernel dot crashing dot org>, "Richard Sandiford" <Richard dot Sandiford at arm dot com>, "GCC Patches" <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 15 Apr 2015 18:45:06 +0800
- Subject: Ping^2 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
- Authentication-results: sourceware.org; auth=none
Ping for trunk?
Hale
> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Friday, February 27, 2015 4:04 AM
> To: Terry Guo
> Cc: Segher Boessenkool; Richard Sandiford; GCC Patches; Hale Wang
> Subject: Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the
insns
> if a volatile register is contained.
>
> Terry Guo <flameroc@gmail.com> writes:
> > On Thu, Feb 26, 2015 at 1:55 PM, Segher Boessenkool
> > <segher@kernel.crashing.org> 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
> >>> <segher@kernel.crashing.org> 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