[RFC PATCH] Handle sequence in reg_set_p

[RFC PATCH] Handle sequence in reg_set_p

[Oleg Endo]

Hi,

Currently reg_set_p doesn't handle sequence rtx, which I've identified
as the root cause of PR 64479.  There is another alternative fix for the
PR, but I'd like to get some comments regarding letting reg_set_p also
handle sequence rtx:

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 218544)
+++ gcc/rtlanal.c	(working copy)
@@ -875,17 +875,24 @@
 {
   /* We can be passed an insn or part of one.  If we are passed an insn,
      check if a side-effect of the insn clobbers REG.  */
-  if (INSN_P (insn)
-      && (FIND_REG_INC_NOTE (insn, reg)
-	  || (CALL_P (insn)
-	      && ((REG_P (reg)
-		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
-		   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
-					       GET_MODE (reg), REGNO (reg)))
-		  || MEM_P (reg)
-		  || find_reg_fusage (insn, CLOBBER, reg)))))
-    return 1;
+  if (INSN_P (insn) && FIND_REG_INC_NOTE (insn, reg))
+    return true;
 
+  /* After delay slot handling, call and branch insns might be in a
+     sequence.  Check all the elements there.  */
+  if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE)
+    for (int i = 0; i < XVECLEN (PATTERN (insn), 0); ++i)
+      if (reg_set_p (reg, XVECEXP (PATTERN (insn), 0, i)))
+	return true;
+
+  if (CALL_P (insn)
+      && ((REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER
+	   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
+				       GET_MODE (reg), REGNO (reg)))
+	  || MEM_P (reg)
+	  || find_reg_fusage (insn, CLOBBER, reg)))
+	return true;
+
   return set_of (reg, insn) != NULL_RTX;
 }
 

Would that be OK to do if it passes testing, or is there something that
could potentially/obviously blow up?

Cheers,
Oleg

Re: [RFC PATCH] Handle sequence in reg_set_p

[Jeff Law]

On 01/08/15 05:23, Oleg Endo wrote:
> Hi,
>
> Currently reg_set_p doesn't handle sequence rtx, which I've identified
> as the root cause of PR 64479.  There is another alternative fix for the
> PR, but I'd like to get some comments regarding letting reg_set_p also
> handle sequence rtx:
>
> Index: gcc/rtlanal.c
> ===================================================================
> --- gcc/rtlanal.c	(revision 218544)
> +++ gcc/rtlanal.c	(working copy)
> @@ -875,17 +875,24 @@
>   {
>     /* We can be passed an insn or part of one.  If we are passed an insn,
>        check if a side-effect of the insn clobbers REG.  */
> -  if (INSN_P (insn)
> -      && (FIND_REG_INC_NOTE (insn, reg)
> -	  || (CALL_P (insn)
> -	      && ((REG_P (reg)
> -		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
> -		   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
> -					       GET_MODE (reg), REGNO (reg)))
> -		  || MEM_P (reg)
> -		  || find_reg_fusage (insn, CLOBBER, reg)))))
> -    return 1;
> +  if (INSN_P (insn) && FIND_REG_INC_NOTE (insn, reg))
> +    return true;
>
> +  /* After delay slot handling, call and branch insns might be in a
> +     sequence.  Check all the elements there.  */
> +  if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE)
> +    for (int i = 0; i < XVECLEN (PATTERN (insn), 0); ++i)
> +      if (reg_set_p (reg, XVECEXP (PATTERN (insn), 0, i)))
> +	return true;
> +
> +  if (CALL_P (insn)
> +      && ((REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER
> +	   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
> +				       GET_MODE (reg), REGNO (reg)))
> +	  || MEM_P (reg)
> +	  || find_reg_fusage (insn, CLOBBER, reg)))
> +	return true;
> +
>     return set_of (reg, insn) != NULL_RTX;
>   }
>
>
> Would that be OK to do if it passes testing, or is there something that
> could potentially/obviously blow up?
It looks reasonable to me.

Any particular reason why the SEQUENCE handling isn't done first, then 
the REG_INC and CALL insn handling?  I'd probably explicitly return 
false if we had a sequence and none of its elements returned true. 
There's no need to check anything on the toplevel SEQUENCE to the best 
of my knowledge.

jeff

Re: [RFC PATCH] Handle sequence in reg_set_p

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]

On Thu, 2015-01-08 at 14:18 -0700, Jeff Law wrote:
> On 01/08/15 05:23, Oleg Endo wrote:
> > Hi,
> >
> > Currently reg_set_p doesn't handle sequence rtx, which I've identified
> > as the root cause of PR 64479.  There is another alternative fix for the
> > PR, but I'd like to get some comments regarding letting reg_set_p also
> > handle sequence rtx:
> >
> > Index: gcc/rtlanal.c
> > ===================================================================
> > --- gcc/rtlanal.c	(revision 218544)
> > +++ gcc/rtlanal.c	(working copy)
> > @@ -875,17 +875,24 @@
> >   {
> >     /* We can be passed an insn or part of one.  If we are passed an insn,
> >        check if a side-effect of the insn clobbers REG.  */
> > -  if (INSN_P (insn)
> > -      && (FIND_REG_INC_NOTE (insn, reg)
> > -	  || (CALL_P (insn)
> > -	      && ((REG_P (reg)
> > -		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
> > -		   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
> > -					       GET_MODE (reg), REGNO (reg)))
> > -		  || MEM_P (reg)
> > -		  || find_reg_fusage (insn, CLOBBER, reg)))))
> > -    return 1;
> > +  if (INSN_P (insn) && FIND_REG_INC_NOTE (insn, reg))
> > +    return true;
> >
> > +  /* After delay slot handling, call and branch insns might be in a
> > +     sequence.  Check all the elements there.  */
> > +  if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE)
> > +    for (int i = 0; i < XVECLEN (PATTERN (insn), 0); ++i)
> > +      if (reg_set_p (reg, XVECEXP (PATTERN (insn), 0, i)))
> > +	return true;
> > +
> > +  if (CALL_P (insn)
> > +      && ((REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER
> > +	   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
> > +				       GET_MODE (reg), REGNO (reg)))
> > +	  || MEM_P (reg)
> > +	  || find_reg_fusage (insn, CLOBBER, reg)))
> > +	return true;
> > +
> >     return set_of (reg, insn) != NULL_RTX;
> >   }
> >
> >
> > Would that be OK to do if it passes testing, or is there something that
> > could potentially/obviously blow up?
> It looks reasonable to me.
> 
> Any particular reason why the SEQUENCE handling isn't done first, then 
> the REG_INC and CALL insn handling?  I'd probably explicitly return 
> false if we had a sequence and none of its elements returned true. 
> There's no need to check anything on the toplevel SEQUENCE to the best 
> of my knowledge.

No meaningful reason.  Attached is an updated patch that applies on 4.8
and trunk.  Bootstrapped on trunk i686-pc-linux-gnu.  Tested with make
-k check RUNTESTFLAGS="--target_board=sh-sim
\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}" on trunk
sh-elf.  In the PR it has been reported, that the patch fixes the
problems when building a native SH GCC.

Although the related SH problem occurs only on 4.8, I'd like to install
this on trunk and 4.9, too, to avoid future surprises.  OK?

Cheers,
Oleg

gcc/ChangeLog:
	PR target/64479
	* rtlanal.c (set_reg_p): Handle SEQUENCE constructs.


[-- Attachment #2: pr64479_1.patch --]
[-- Type: text/x-patch, Size: 951 bytes --]

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 218544)
+++ gcc/rtlanal.c	(working copy)
@@ -873,6 +873,17 @@
 int
 reg_set_p (const_rtx reg, const_rtx insn)
 {
+  /* After delay slot handling, call and branch insns might be in a
+     sequence.  Check all the elements there.  */
+  if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE)
+    {
+      for (int i = 0; i < XVECLEN (PATTERN (insn), 0); ++i)
+	if (reg_set_p (reg, XVECEXP (PATTERN (insn), 0, i)))
+	  return true;
+
+      return false;
+    }
+
   /* We can be passed an insn or part of one.  If we are passed an insn,
      check if a side-effect of the insn clobbers REG.  */
   if (INSN_P (insn)
@@ -884,7 +895,7 @@
 					       GET_MODE (reg), REGNO (reg)))
 		  || MEM_P (reg)
 		  || find_reg_fusage (insn, CLOBBER, reg)))))
-    return 1;
+    return true;
 
   return set_of (reg, insn) != NULL_RTX;
 }

Re: [RFC PATCH] Handle sequence in reg_set_p

[Jeff Law]

On 01/11/15 04:40, Oleg Endo wrote:
>>
>> Any particular reason why the SEQUENCE handling isn't done first, then
>> the REG_INC and CALL insn handling?  I'd probably explicitly return
>> false if we had a sequence and none of its elements returned true.
>> There's no need to check anything on the toplevel SEQUENCE to the best
>> of my knowledge.
>
> No meaningful reason.  Attached is an updated patch that applies on 4.8
> and trunk.  Bootstrapped on trunk i686-pc-linux-gnu.  Tested with make
> -k check RUNTESTFLAGS="--target_board=sh-sim
> \{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}" on trunk
> sh-elf.  In the PR it has been reported, that the patch fixes the
> problems when building a native SH GCC.
>
> Although the related SH problem occurs only on 4.8, I'd like to install
> this on trunk and 4.9, too, to avoid future surprises.  OK?
>
> Cheers,
> Oleg
>
> gcc/ChangeLog:
> 	PR target/64479
> 	* rtlanal.c (set_reg_p): Handle SEQUENCE constructs.
OK.

Jeff