[h8300] misc patches

[h8300] misc patches

[DJ Delorie]

Two misc patches here.  The first solves this build failure:

gcc/unwind-dw2-fde.c:146: error: unrecognizable insn:
(insn 17 16 18 2gcc/unwind-dw2-fde.c:136 (set (zero_extract:HI (subreg:HI (reg:QI 23) 0)
            (const_int 1 [0x1])
            (const_int 6 [0x6]))
        (const_int 1 [0x1])) -1 (nil))
../gcc/unwind-dw2-fde.c:146: internal compiler error: in extract_insn, at recog.c:1990

	* config/h8300/h8300.md (insv): Allow constants under the
	assumption that they'll get moved to a register later.

Index: h8300.md
===================================================================
--- h8300.md	(revision 131405)
+++ h8300.md	(working copy)
@@ -3285,13 +3285,13 @@
 }")
 
 (define_insn ""
   [(set (zero_extract:HI (match_operand:HI 0 "register_operand" "+r")
 			 (const_int 1)
 			 (match_operand:HI 1 "immediate_operand" "n"))
-	(match_operand:HI 2 "register_operand" "r"))]
+	(match_operand:HI 2 "general_operand" "r"))]
   ""
   "bld	#0,%R2\;bst	%Z1,%Y0 ; i1"
   [(set_attr "length" "4")])
 
 (define_expand "extzv"
   [(set (match_operand:HI 0 "register_operand" "")


This second one solves numerous testsuite failures, as the return insn
in some functions was being deleted during optimization.

	* config/h8300/h8300.c (h8300_expand_epilogue): Return insns
	must be emitted as jumps.

Index: h8300.c
===================================================================
--- h8300.c	(revision 131405)
+++ h8300.c	(working copy)
@@ -927,13 +927,13 @@ h8300_expand_epilogue (void)
       if (TARGET_H8300SX)
 	returned_p = true;
       h8300_push_pop (HARD_FRAME_POINTER_REGNUM, 1, 1, returned_p);
     }
 
   if (!returned_p)
-    emit_insn (gen_rtx_RETURN (VOIDmode));
+    emit_jump_insn (gen_rtx_RETURN (VOIDmode));
 }
 
 /* Return nonzero if the current function is an interrupt
    function.  */
 
 int

Re: [h8300] misc patches

[Hans-Peter Nilsson]

On Mon, 21 Jan 2008, DJ Delorie wrote:
> Two misc patches here.  The first solves this build failure:
>
> gcc/unwind-dw2-fde.c:146: error: unrecognizable insn:
> (insn 17 16 18 2gcc/unwind-dw2-fde.c:136 (set (zero_extract:HI (subreg:HI (reg:QI 23) 0)
>             (const_int 1 [0x1])
>             (const_int 6 [0x6]))
>         (const_int 1 [0x1])) -1 (nil))
> ../gcc/unwind-dw2-fde.c:146: internal compiler error: in extract_insn, at recog.c:1990
>
> 	* config/h8300/h8300.md (insv): Allow constants under the
> 	assumption that they'll get moved to a register later.
>
> Index: h8300.md
> ===================================================================
> --- h8300.md	(revision 131405)
> +++ h8300.md	(working copy)
> @@ -3285,13 +3285,13 @@
>  }")
>
>  (define_insn ""
>    [(set (zero_extract:HI (match_operand:HI 0 "register_operand" "+r")
>  			 (const_int 1)
>  			 (match_operand:HI 1 "immediate_operand" "n"))
> -	(match_operand:HI 2 "register_operand" "r"))]
> +	(match_operand:HI 2 "general_operand" "r"))]
>    ""

Sorry, can't help but saying: without a comment, this looks ripe
for a cleanup, reverting the change; perhaps successfully that
time, more or less temporarily.  You already know of course that
this is just papering over a bug elsewhere, so no point
in mentioning that(*). :)

(*) By a quick look, seems like a missing force_reg on
operands[2] in the insv define_expand, but I might be missing
something or two.

brgds, H-P

Re: [h8300] misc patches

[DJ Delorie]

> Sorry, can't help but saying: without a comment, this looks ripe
> for a cleanup, reverting the change; perhaps successfully that
> time, more or less temporarily.  You already know of course that
> this is just papering over a bug elsewhere, so no point
> in mentioning that(*). :)
> 
> (*) By a quick look, seems like a missing force_reg on
> operands[2] in the insv define_expand, but I might be missing
> something or two.

I worried about that, but I've been fixing a LOT of bugs in various
ports (esp m32c) by making the predicates more general when the
constraints can include a general register, even in cases without an
expander IIRC.  Alloc/reload seem to like it better that way.

I can only imagine that there's some paradigm shift going on
somewhere.  Or I'm going slowly insane.  Either way it will be
entertaining.

Re: [h8300] misc patches

[DJ Delorie]

> (*) By a quick look, seems like a missing force_reg on
> operands[2] in the insv define_expand, but I might be missing
> something or two.

That seems to work also, although the logic just before it implies
that they're expecting non-register operands.

Index: h8300.md
===================================================================
--- h8300.md	(revision 131405)
+++ h8300.md	(working copy)
@@ -3279,12 +3279,15 @@
     FAIL;
 
   /* For now, we don't allow memory operands.  */
   if (GET_CODE (operands[0]) == MEM
       || GET_CODE (operands[3]) == MEM)
     FAIL;
+
+  if (GET_CODE (operands[3]) != REG)
+    operands[3] = force_reg (HImode, operands[3]);
 }")
 
 (define_insn ""
   [(set (zero_extract:HI (match_operand:HI 0 "register_operand" "+r")
 			 (const_int 1)
 			 (match_operand:HI 1 "immediate_operand" "n"))

Re: [h8300] misc patches

[Hans-Peter Nilsson]

On Wed, 23 Jan 2008, DJ Delorie wrote:
> > (*) By a quick look, seems like a missing force_reg on
> > operands[2] in the insv define_expand, but I might be missing
> > something or two.
>
> That seems to work also, although the logic just before it implies
> that they're expecting non-register operands.

Sorry to not see it sooner; it seems just the predicate of
operand[2] of the "insv" define_expand should be changed from
general_operand to register_operand.

>
> Index: h8300.md
> ===================================================================
> --- h8300.md	(revision 131405)
> +++ h8300.md	(working copy)
> @@ -3279,12 +3279,15 @@
>      FAIL;
>
>    /* For now, we don't allow memory operands.  */
>    if (GET_CODE (operands[0]) == MEM
>        || GET_CODE (operands[3]) == MEM)
>      FAIL;
> +
> +  if (GET_CODE (operands[3]) != REG)

Never to miss a one-upping opportunity, that test is redundant
(and these days written REG_P), see force_reg.  The below would
been sufficient.  Except that using register_operand as
predicate would be even shorter.

> +    operands[3] = force_reg (HImode, operands[3]);
>  }")

brgds, H-P

Re: [h8300] misc patches

[DJ Delorie]

> Sorry to not see it sooner; it seems just the predicate of
> operand[2] of the "insv" define_expand should be changed from
> general_operand to register_operand.

Seems to work too.  Current patch:

Index: h8300.c
===================================================================
--- h8300.c	(revision 131405)
+++ h8300.c	(working copy)
@@ -927,13 +927,13 @@ h8300_expand_epilogue (void)
       if (TARGET_H8300SX)
 	returned_p = true;
       h8300_push_pop (HARD_FRAME_POINTER_REGNUM, 1, 1, returned_p);
     }
 
   if (!returned_p)
-    emit_insn (gen_rtx_RETURN (VOIDmode));
+    emit_jump_insn (gen_rtx_RETURN (VOIDmode));
 }
 
 /* Return nonzero if the current function is an interrupt
    function.  */
 
 int
Index: h8300.md
===================================================================
--- h8300.md	(revision 131405)
+++ h8300.md	(working copy)
@@ -3222,13 +3222,13 @@
    (set_attr "length" "8,6")])
 
 (define_expand "insv"
   [(set (zero_extract:HI (match_operand:HI 0 "general_operand" "")
 			 (match_operand:HI 1 "general_operand" "")
 			 (match_operand:HI 2 "general_operand" ""))
-	(match_operand:HI 3 "general_operand" ""))]
+	(match_operand:HI 3 "register_operand" ""))]
   "TARGET_H8300 || TARGET_H8300SX"
   "
 {
   if (TARGET_H8300SX)
     {
       if (GET_CODE (operands[1]) == CONST_INT

Re: [h8300] misc patches

[DJ Delorie]

Now that we've missed the cutoff for getting a working H8/300 port
in 4.3, can we revisit these patches?

http://gcc.gnu.org/ml/gcc-patches/2008-01/msg00973.html
http://gcc.gnu.org/ml/gcc-patches/2008-01/msg01122.html

Re: [h8300] misc patches

[Jeff Law]

DJ Delorie wrote:
> Now that we've missed the cutoff for getting a working H8/300 port
> in 4.3, can we revisit these patches?
> 
> http://gcc.gnu.org/ml/gcc-patches/2008-01/msg00973.html
> http://gcc.gnu.org/ml/gcc-patches/2008-01/msg01122.html
The hunk to change emit_insn to emit_jump_insn is obviously correct.
It wouldn't be a terrible idea to do a grep across all the backends
for this buglet -- I know I've fixed the exact same problem once or
twice in other ports.

My H8 is very rusty, but I don't think the target supports anything
except a  register for that operand.  So I think we're better off
tightening the expander rather than loosening the matching pattern.
It looks like one variant of the patch does that.  Assuming it's
passed testing, I'm good with it.
jeff

Re: [h8300] misc patches

[DJ Delorie]

> It looks like one variant of the patch does that.  Assuming it's
> passed testing, I'm good with it.

Thanks.  When (if ever) can these patches go into 4.3?  libgcc doesn't
compile without them.

Re: [h8300] misc patches

[Jeff Law]

DJ Delorie wrote:
>> It looks like one variant of the patch does that.  Assuming it's
>> passed testing, I'm good with it.
> 
> Thanks.  When (if ever) can these patches go into 4.3?  libgcc doesn't
> compile without them.
It's up to the release managers.  I'd consider the fix for return insns
a no-brainer.  The insv change is marginally riskier, but I'd still
consider it very low risk.  Both would have my support for inclusion
into release branches.

Jeff

Re: [h8300] misc patches

[Jakub Jelinek]

On Tue, Feb 19, 2008 at 02:26:22PM -0500, DJ Delorie wrote:
> 
> > It looks like one variant of the patch does that.  Assuming it's
> > passed testing, I'm good with it.
> 
> Thanks.  When (if ever) can these patches go into 4.3?  libgcc doesn't
> compile without them.

If libgcc doesn't compile without them and you have the patches tested or
can do the testing today and Jeff is ok with them, I think they can be
applied now to 4.3 branch.  Rationale is that gcc/config/h8300/ confined
patches won't affect other targets and if they make a non-buildable target
buildable, it can only make 4.3 a better release.  But we of course won't
delay 4.3 for it.

We are still waiting to get COMMAND //c doc stuff resolved on mingw host
and ppc32-linux needs the altivec ABI changes.

	Jakub

Re: [h8300] misc patches

[DJ Delorie]

> If libgcc doesn't compile without them and you have the patches
> tested or can do the testing today and Jeff is ok with them, I think
> they can be applied now to 4.3 branch.

Yup, it won't build without them, and passes about 99% of the C
testsuite with them.  Committed as follows, to the 4.3 branch and trunk:

2008-02-20  DJ Delorie  <dj@redhat.com>

	* config/h8300/h8300.md (insv): Force source operand to be a register.

	* config/h8300/h8300.c (h8300_expand_epilogue): Emit return insn
	as a jump, not as a plain insn.
	
Index: config/h8300/h8300.c
===================================================================
--- config/h8300/h8300.c	(revision 132496)
+++ config/h8300/h8300.c	(working copy)
@@ -930,7 +930,7 @@
     }
 
   if (!returned_p)
-    emit_insn (gen_rtx_RETURN (VOIDmode));
+    emit_jump_insn (gen_rtx_RETURN (VOIDmode));
 }
 
 /* Return nonzero if the current function is an interrupt
Index: config/h8300/h8300.md
===================================================================
--- config/h8300/h8300.md	(revision 132496)
+++ config/h8300/h8300.md	(working copy)
@@ -3282,6 +3282,9 @@
   if (GET_CODE (operands[0]) == MEM
       || GET_CODE (operands[3]) == MEM)
     FAIL;
+
+  if (GET_CODE (operands[3]) != REG)
+    operands[3] = force_reg (HImode, operands[3]);
 }")
 
 (define_insn ""