Patchwork clean up pdp11.md a bit

login
register
mail settings
Submitter Steven Bosscher
Date Feb. 14, 2012, 10:08 p.m.
Message ID <CABu31nPp0iY-GDyO8NXdZ3f-_BtU8vVGjAyCRLAvWuD5uVHQuA@mail.gmail.com>
Download mbox | patch
Permalink /patch/141207/
State New
Headers show

Comments

Steven Bosscher - Feb. 14, 2012, 10:08 p.m.
Hello,

Just a few cleanups for things I noticed last weekend.

* Constraints on define_expand are never used, remove them (most other
ports do not have constraints on define_expands either)
* Some patterns have no name, makes debugging a bit harder
* The divmodhi4 expander has been commented out since Day 1 of egcs.
Explain why.

Tested by building the compiler. For pdp11-elf, which is not a
supported target according to backends.html, but it's not rejected.
Not sure what the right target is to compile for. In any case, I
obviously have no PDP-11 to test on. But the patch has no changes that
impact code generation (the constraints were already ignored).

OK for trunk?

Ciao!
Steven
* pdp11.md: Add names to some patterns. Clean up constraints
	on define_expands.  Explain why divmodhi4 is commented out.
Richard Henderson - Feb. 14, 2012, 10:51 p.m.
On 02/14/2012 02:08 PM, Steven Bosscher wrote:
> OK for trunk?

This can wait for stage1.

> +;; On PDP-11, DIV always produces a quotient and a remainder.  But CSE
> +;; cannot optimize the divmods away because the SET_DESTs are SUBREGs.
> +;
>  ;(define_expand "divmodhi4"
>  ;  [(parallel [(set (subreg:HI (match_dup 1) 0)

Which of course begs the question of why that's so.
The division patterns can be modeled similarly to s390.

Indeed, all of the appearances of subreg in the md file are errors:

Should be removed as unnecessary:

  (define_expand "truncsihi2"

Should be removed as generated by generic code:

  (define_insn "zero_extendqihi2"
  (define_expand "zero_extendhisi2"


r~
Steven Bosscher - Feb. 14, 2012, 11:41 p.m.
On Tue, Feb 14, 2012 at 11:51 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/14/2012 02:08 PM, Steven Bosscher wrote:
>> OK for trunk?
>
> This can wait for stage1.
>
>> +;; On PDP-11, DIV always produces a quotient and a remainder.  But CSE
>> +;; cannot optimize the divmods away because the SET_DESTs are SUBREGs.
>> +;
>>  ;(define_expand "divmodhi4"
>>  ;  [(parallel [(set (subreg:HI (match_dup 1) 0)
>
> Which of course begs the question of why that's so.
> The division patterns can be modeled similarly to s390.

That is what Ian also said on IRC, but I didn't want to go that far (I
didn't think this basically cosmetic change would be a problem for
stage4 but rewriting patterns, even for a pet target like this, didn't
seem appropriate).

Anyway, history for this goes back a long long time:
http://gcc.gnu.org/viewcvs?view=revision&revision=10175

The previous div/mod/divmod patterns didn't have subregs. For example:

 (define_expand "divhi3"
-  [(set (match_dup 3)
-	(sign_extend:SI (match_operand:HI 1 "general_operand" "g")))
-   (set (match_operand:HI 0 "general_operand" "g")
-	(truncate:HI
-	 (div:SI
-	  (match_dup 3)
-	  (sign_extend:SI (match_operand:HI 2 "general_operand" "g")))))]
-  "TARGET_45"
-  "operands[3] = gen_reg_rtx (SImode);")

Not sure why this was changed. What I'd like to do, is remove the
divhi3 and movhi3 define_expand and define_insn, and just keep
divmodhi4. You point to s390. I suppose you mean the divmoddi4
expander there?


> Indeed, all of the appearances of subreg in the md file are errors:

I was only playing with your match_flags patches. I get the feeling I
am going to regret that... :-)


> Should be removed as unnecessary:
>
>  (define_expand "truncsihi2"
>
> Should be removed as generated by generic code:
>
>  (define_insn "zero_extendqihi2"
>  (define_expand "zero_extendhisi2"

What do you mean with "generated by generic code"?
And the define_insn has to stay, surely?

Thanks, and sorry for the novice questions!

Ciao!
Steven
Richard Henderson - Feb. 15, 2012, 12:07 a.m.
On 02/14/2012 03:41 PM, Steven Bosscher wrote:
> Not sure why this was changed. What I'd like to do, is remove the
> divhi3 and movhi3 define_expand and define_insn, and just keep
> divmodhi4. You point to s390. I suppose you mean the divmoddi4
> expander there?

Yes.  Naturally the TImode stuff becomes SImode in the pdp11 version.

>>  (define_insn "zero_extendqihi2"
>>  (define_expand "zero_extendhisi2"
> 
> What do you mean with "generated by generic code"?
> And the define_insn has to stay, surely?

I mean that optabs.c will generate the right thing by itself
without you providing these patterns at all.


r~
Paul Koning - Feb. 16, 2012, 5:25 p.m.
These look fine.  I'll defer to others on whether it should wait to Phase 1.

I had tried to make divmod work but never figured out the reason why it did not.  Thanks for answering that question.

As for the subregs that Richard commented on -- I will gladly admit that this target isn't all clean yet.  So that goes onto the list of things to work on, too.

	paul

-----Original Message-----
From: Steven Bosscher [mailto:stevenb.gcc@gmail.com] 
Sent: Tuesday, February 14, 2012 5:09 PM
To: Koning, Paul; GCC Patches
Subject: [patch] clean up pdp11.md a bit

Hello,

Just a few cleanups for things I noticed last weekend.

* Constraints on define_expand are never used, remove them (most other ports do not have constraints on define_expands either)
* Some patterns have no name, makes debugging a bit harder
* The divmodhi4 expander has been commented out since Day 1 of egcs.
Explain why.

Tested by building the compiler. For pdp11-elf, which is not a supported target according to backends.html, but it's not rejected.
Not sure what the right target is to compile for. In any case, I obviously have no PDP-11 to test on. But the patch has no changes that impact code generation (the constraints were already ignored).

OK for trunk?

Ciao!
Steven
Paul Koning - Feb. 21, 2012, 4:09 p.m.
By the way, the "compile" subset of the testsuite works for pdp11; it has some errors which still need cleanup but a large fraction works.

	paul

-----Original Message-----
From: Steven Bosscher [mailto:stevenb.gcc@gmail.com] 
Sent: Tuesday, February 14, 2012 5:09 PM
To: Koning, Paul; GCC Patches
Subject: [patch] clean up pdp11.md a bit

Hello,

Just a few cleanups for things I noticed last weekend.

* Constraints on define_expand are never used, remove them (most other ports do not have constraints on define_expands either)
* Some patterns have no name, makes debugging a bit harder
* The divmodhi4 expander has been commented out since Day 1 of egcs.
Explain why.

Tested by building the compiler. For pdp11-elf, which is not a supported target according to backends.html, but it's not rejected.
Not sure what the right target is to compile for. In any case, I obviously have no PDP-11 to test on. But the patch has no changes that impact code generation (the constraints were already ignored).

OK for trunk?

Ciao!
Steven

Patch

Index: pdp11.md
===================================================================
--- pdp11.md	(revision 184217)
+++ pdp11.md	(working copy)
@@ -1,6 +1,6 @@ 
 ;;- Machine description for the pdp11 for GNU C compiler
 ;; Copyright (C) 1994, 1995, 1997, 1998, 1999, 2000, 2001, 2004, 2005
-;; 2007, 2008, 2010 Free Software Foundation, Inc.
+;; 2007, 2008, 2010, 2012 Free Software Foundation, Inc.
 ;; Contributed by Michael K. Gschwind (mike@vlsivie.tuwien.ac.at).
 
 ;; This file is part of GCC.
@@ -195,7 +195,7 @@ 
 ;; sob instruction - we need an assembler which can make this instruction
 ;; valid under _all_ circumstances!
 
-(define_insn ""
+(define_insn "*sob"
   [(set (pc)
 	(if_then_else
 	 (ne (plus:HI (match_operand:HI 0 "register_operand" "+r")
@@ -242,8 +242,8 @@ 
 
 (define_expand "cbranchdf4"
   [(set (cc0)
-        (compare (match_operand:DF 1 "general_operand")
-		 (match_operand:DF 2 "register_or_const0_operand")))
+        (compare (match_operand:DF 1 "general_operand" "")
+		 (match_operand:DF 2 "register_or_const0_operand" "")))
    (set (pc)
 	(if_then_else (match_operator 0 "ordered_comparison_operator"
 		       [(cc0) (const_int 0)])
@@ -254,8 +254,8 @@ 
 
 (define_expand "cbranch<mode>4"
   [(set (cc0)
-        (compare (match_operand:PDPint 1 "general_operand")
-		 (match_operand:PDPint 2 "general_operand")))
+        (compare (match_operand:PDPint 1 "general_operand" "")
+		 (match_operand:PDPint 2 "general_operand" "")))
    (set (pc)
 	(if_then_else (match_operator 0 "ordered_comparison_operator"
 		       [(cc0) (const_int 0)])
@@ -370,11 +370,11 @@ 
 ;; let constraints only accept a register ...
 
 (define_expand "movmemhi"
-  [(parallel [(set (match_operand:BLK 0 "general_operand" "=g,g")
-		   (match_operand:BLK 1 "general_operand" "g,g"))
-	      (use (match_operand:HI 2 "general_operand" "n,mr"))
-	      (use (match_operand:HI 3 "immediate_operand" "i,i"))
-	      (clobber (match_scratch:HI 4 "=&r,X"))
+  [(parallel [(set (match_operand:BLK 0 "general_operand" "")
+		   (match_operand:BLK 1 "general_operand" ""))
+	      (use (match_operand:HI 2 "general_operand" ""))
+	      (use (match_operand:HI 3 "immediate_operand" ""))
+	      (clobber (match_scratch:HI 4 ""))
 	      (clobber (match_dup 5))
 	      (clobber (match_dup 6))
 	      (clobber (match_dup 2))])]
@@ -428,9 +428,9 @@ 
 
 
 (define_expand "truncsihi2"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=g")
+  [(set (match_operand:HI 0 "nonimmediate_operand" "")
 	(subreg:HI 
-	  (match_operand:SI 1 "general_operand" "or")
+	  (match_operand:SI 1 "general_operand" "")
           0))]
   ""
   "")
@@ -449,9 +449,9 @@ 
   [(set (subreg:HI 
           (match_dup 0)
           2)
-        (match_operand:HI 1 "register_operand" "r"))
+        (match_operand:HI 1 "register_operand" ""))
    (set (subreg:HI 
-          (match_operand:SI 0 "register_operand" "=r")
+          (match_operand:SI 0 "register_operand" "")
           0)
         (const_int 0))]
   ""
@@ -501,12 +501,12 @@ 
 ;; unconditionally, and then match dependent on CPU type:
 
 (define_expand "extendhisi2"
-  [(set (match_operand:SI 0 "nonimmediate_operand" "=g")
-	(sign_extend:SI (match_operand:HI 1 "general_operand" "g")))]
+  [(set (match_operand:SI 0 "nonimmediate_operand" "")
+	(sign_extend:SI (match_operand:HI 1 "general_operand" "")))]
   ""
   "")
   
-(define_insn "" ; "extendhisi2"
+(define_insn "*extendhisi2_40"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=o,<,r")
 	(sign_extend:SI (match_operand:HI 1 "general_operand" "g,g,g")))]
   "TARGET_40_PLUS"
@@ -555,7 +555,7 @@ 
   [(set_attr "length" "10,6,6")])
 
 
-(define_insn ""
+(define_insn "*extendhisi2_not40"
   [(set (match_operand:SI 0 "register_operand" "=r")
 	(sign_extend:SI (match_operand:HI 1 "general_operand" "0")))]
   "(! TARGET_40_PLUS)"
@@ -926,9 +926,9 @@ 
 
 ;; Arithmetic right shift on the pdp works by negating the shift count.
 (define_expand "ashrsi3"
-  [(set (match_operand:SI 0 "register_operand" "=r")
-	(ashift:SI (match_operand:SI 1 "register_operand" "0")
-		   (match_operand:HI 2 "general_operand" "g")))]
+  [(set (match_operand:SI 0 "register_operand" "")
+	(ashift:SI (match_operand:SI 1 "register_operand" "")
+		   (match_operand:HI 2 "general_operand" "")))]
   ""
   "
 {
@@ -993,7 +993,7 @@ 
 
 (define_expand "lshrsi3"
   [(match_operand:SI 0 "register_operand" "")
-   (match_operand:SI 1 "register_operand" "0")
+   (match_operand:SI 1 "register_operand" "")
    (match_operand:HI 2 "general_operand" "")]
   ""
   "
@@ -1128,9 +1128,9 @@ 
 
 ;; Arithmetic right shift on the pdp works by negating the shift count.
 (define_expand "ashrhi3"
-  [(set (match_operand:HI 0 "register_operand" "=r")
-	(ashift:HI (match_operand:HI 1 "register_operand" "0")
-		   (match_operand:HI 2 "general_operand" "g")))]
+  [(set (match_operand:HI 0 "register_operand" "")
+	(ashift:HI (match_operand:HI 1 "register_operand" "")
+		   (match_operand:HI 2 "general_operand" "")))]
   ""
   "
 {
@@ -1254,7 +1254,7 @@ 
 				      (const_int 4)
 				      (const_int 2)))])
 
-(define_insn ""
+(define_insn "*jump"
   [(set (pc)
     (label_ref (match_operand 0 "" "")))
    (clobber (const_int 1))]
@@ -1339,15 +1339,15 @@ 
 ;; 32 bit result
 (define_expand "mulhisi3"
   [(set (match_dup 3)
-	(match_operand:HI 1 "nonimmediate_operand" "g,g"))
-   (set (match_operand:SI 0 "register_operand" "=r,r") ; even numbered!
+	(match_operand:HI 1 "nonimmediate_operand" ""))
+   (set (match_operand:SI 0 "register_operand" "")
 	(mult:SI (truncate:HI 
                   (match_dup 0))
-		 (match_operand:HI 2 "general_operand" "rR,Qi")))]
+		 (match_operand:HI 2 "general_operand" "")))]
   "TARGET_40_PLUS"
   "operands[3] = gen_lowpart(HImode, operands[1]);")
 
-(define_insn ""
+(define_insn "*mulhisi3"
   [(set (match_operand:SI 0 "register_operand" "=r,r") ; even numbered!
 	(mult:SI (truncate:HI 
                   (match_operand:SI 1 "register_operand" "%0,0"))
@@ -1384,7 +1384,7 @@ 
   "TARGET_40_PLUS"
   "")
 
-(define_insn ""
+(define_insn "*divhi3"
   [(set (subreg:HI (match_operand:SI 0 "register_operand" "=r") 0)
 	(div:HI (match_operand:SI 1 "general_operand" "0")
 		(match_operand:HI 2 "general_operand" "g")))]
@@ -1401,7 +1401,7 @@ 
   "TARGET_40_PLUS"
   "")
 
-(define_insn ""
+(define_insn "*modhi3"
   [(set (subreg:HI (match_operand:SI 0 "register_operand" "=r") 2)
 	(mod:HI (match_operand:SI 1 "general_operand" "0")
 		(match_operand:HI 2 "general_operand" "g")))]
@@ -1409,6 +1409,19 @@ 
   "div %2,%0"
   [(set_attr "length" "4")])
 
+;;
+;; FIXME
+;; From the GCC internals manual:
+;;
+;; "For machines with an instruction that produces both a quotient and
+;;  a remainder, provide a pattern for `divmodm4' but do not provide
+;;  patterns for `divm3' and `modm3'. This allows optimization in the
+;;  relatively common case when both the quotient and remainder are
+;;  computed."
+;;
+;; On PDP-11, DIV always produces a quotient and a remainder.  But CSE
+;; cannot optimize the divmods away because the SET_DESTs are SUBREGs.
+;
 ;(define_expand "divmodhi4"
 ;  [(parallel [(set (subreg:HI (match_dup 1) 0)
 ;	           (div:HI (match_operand:SI 1 "register_operand" "0")