[Patch,AVR]: Solve PR42210

[Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

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

This solves some missed optimization that can be seen when moving
around bits.

There are 4 combiner patterns that operate on regs and one that uses
them as intermediate patterns and works on I/O. Even if just an
intermediate pattern matches it's still an improvement because avoid
of shift.

Tested on some home-brew example.

Ok if I see no regressions?

Johann

2011-04-20  Georg-Johann Lay  <avr@gjlay.de>

	PR target/42210

	* config/avr/avr.md ("*movbitqi.1-6.a", "*movbitqi.1-6.b",
	"*movbitqi.0", "*movbitqi.7", "*movbitqi.io"): New insns.

[-- Attachment #2: pr42210.diff --]
[-- Type: text/x-patch, Size: 4058 bytes --]

Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(Revision 172770)
+++ config/avr/avr.md	(Arbeitskopie)
@@ -3388,3 +3388,81 @@ (define_insn "fmulsu"
 	clr __zero_reg__"
   [(set_attr "length" "3")
    (set_attr "cc" "clobber")])
+
+\f
+;; Some combiner patterns dealing with bits.
+;; See PR42210
+
+;; Move bit $3.$4 into bit $0.$4
+(define_insn "*movbitqi.1-6.a"
+  [(set (match_operand:QI 0 "register_operand"                            "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"             "0")
+                        (match_operand:QI 2 "single_zero_operand"          "n"))
+                (and:QI (ashift:QI (match_operand:QI 3 "register_operand"  "r")
+                                   (match_operand:QI 4 "const_int_operand" "n"))
+                        (match_operand:QI 5 "single_one_operand" "n"))))]
+  "optimize
+   && INTVAL(operands[4]) == exact_log2 (INTVAL(operands[5]) & GET_MODE_MASK (QImode))
+   && INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,%4\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.$4 into bit $0.$4
+;; Variation of above. Unfortunately, there is no canonicalized representation
+;; of moving around bits.  So what we see here depends on how user writes down
+;; bit manipulations.
+(define_insn "*movbitqi.1-6.b"
+  [(set (match_operand:QI 0 "register_operand"                            "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"             "0")
+                        (match_operand:QI 2 "single_zero_operand"          "n"))
+                (ashift:QI (and:QI (match_operand:QI 3 "register_operand"  "r")
+                                   (const_int 1))
+                           (match_operand:QI 4 "const_int_operand"         "n"))))]
+  "optimize
+   && INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,%4\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.0.
+;; For bit 0, combiner generates slightly different pattern.
+(define_insn "*movbitqi.0"
+  [(set (match_operand:QI 0 "register_operand"                     "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"      "0")
+                        (match_operand:QI 2 "single_zero_operand"   "n"))
+                (and:QI (match_operand:QI 3 "register_operand"      "r")
+                        (const_int 1))))]
+  "optimize
+   && 0 == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,0"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $2.7 into bit $0.7.
+;; For bit 7, combiner generates slightly different pattern
+(define_insn "*movbitqi.7"
+  [(set (match_operand:QI 0 "register_operand"                      "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"       "0")
+                        (const_int 127))
+                (ashift:QI (match_operand:QI 2 "register_operand"    "r")
+                           (const_int 7))))]
+  "optimize"
+  "bst %2,7\;bld %0,7"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Combiner transforms above four pattern into ZERO_EXTRACT if it sees MEM
+;; and input/output match.  We provide a special pattern for this, because
+;; in contrast to a IN/BST/BLD/OUT sequence we need less registers and the
+;; operation on I/O is atomic.
+(define_insn "*movbitqi.io"
+  [(set (zero_extract:QI (mem:QI (match_operand 0 "low_io_address_operand" "n"))
+                         (const_int 1)                                           ;; width
+                         (match_operand 1 "const_int_operand"              "n")) ;; pos
+        (match_operand:QI 2 "register_operand"                             "r"))]
+  "optimize
+   && IN_RANGE (INTVAL(operands[1]), 0, 7)"
+  "sbrc %2,0\;sbi %m0-0x20,%1\;sbrs %2,0\;cbi %m0-0x20,%1"
+  [(set_attr "length" "4")
+   (set_attr "cc" "none")])

Re: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

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

Georg-Johann Lay schrieb:
> This solves some missed optimization that can be seen when moving
> around bits.
> 
> There are 4 combiner patterns that operate on regs and one that uses
> them as intermediate patterns and works on I/O. Even if just an
> intermediate pattern matches it's still an improvement because avoid
> of shift.
> 
> Tested on some home-brew example.
> 
> Ok if I see no regressions?

The original patch has a thinko. The source bit is always 0.
Besides that, the (set (zero_extract is now allowed to handle constant
sources, too.

The new patch fixes that and passes testsuite.

Johann

> 2011-04-20  Georg-Johann Lay  <avr@gjlay.de>
> 
> 	PR target/42210
> 
> 	* config/avr/avr.md ("*movbitqi.1-6.a", "*movbitqi.1-6.b",
> 	"*movbitqi.0", "*movbitqi.7", "*movbitqi.io"): New insns.
> 


[-- Attachment #2: pr42210.diff --]
[-- Type: text/x-patch, Size: 4063 bytes --]

Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(Revision 172770)
+++ config/avr/avr.md	(Arbeitskopie)
@@ -3388,3 +3388,84 @@ (define_insn "fmulsu"
 	clr __zero_reg__"
   [(set_attr "length" "3")
    (set_attr "cc" "clobber")])
+
+\f
+;; Some combiner patterns dealing with bits.
+;; See PR42210
+
+;; Move bit $3.$4 into bit $0.$4
+(define_insn "*movbitqi.1-6.a"
+  [(set (match_operand:QI 0 "register_operand"                            "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"             "0")
+                        (match_operand:QI 2 "single_zero_operand"          "n"))
+                (and:QI (ashift:QI (match_operand:QI 3 "register_operand"  "r")
+                                   (match_operand:QI 4 "const_int_operand" "n"))
+                        (match_operand:QI 5 "single_one_operand"           "n"))))]
+  "optimize
+   && INTVAL(operands[4]) == exact_log2 (INTVAL(operands[5]) & GET_MODE_MASK (QImode))
+   && INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.$4
+;; Variation of above. Unfortunately, there is no canonicalized representation
+;; of moving around bits.  So what we see here depends on how user writes down
+;; bit manipulations.
+(define_insn "*movbitqi.1-6.b"
+  [(set (match_operand:QI 0 "register_operand"                            "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"             "0")
+                        (match_operand:QI 2 "single_zero_operand"          "n"))
+                (ashift:QI (and:QI (match_operand:QI 3 "register_operand"  "r")
+                                   (const_int 1))
+                           (match_operand:QI 4 "const_int_operand"         "n"))))]
+  "optimize
+   && INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.0.
+;; For bit 0, combiner generates slightly different pattern.
+(define_insn "*movbitqi.0"
+  [(set (match_operand:QI 0 "register_operand"                     "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"      "0")
+                        (match_operand:QI 2 "single_zero_operand"   "n"))
+                (and:QI (match_operand:QI 3 "register_operand"      "r")
+                        (const_int 1))))]
+  "optimize
+   && 0 == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,0"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $2.0 into bit $0.7.
+;; For bit 7, combiner generates slightly different pattern
+(define_insn "*movbitqi.7"
+  [(set (match_operand:QI 0 "register_operand"                      "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"       "0")
+                        (const_int 127))
+                (ashift:QI (match_operand:QI 2 "register_operand"    "r")
+                           (const_int 7))))]
+  "optimize"
+  "bst %2,0\;bld %0,7"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Combiner transforms above four pattern into ZERO_EXTRACT if it sees MEM
+;; and input/output match.  We provide a special pattern for this, because
+;; in contrast to a IN/BST/BLD/OUT sequence we need less registers and the
+;; operation on I/O is atomic.
+(define_insn "*movbitqi.io"
+  [(set (zero_extract:QI (mem:QI (match_operand 0 "low_io_address_operand" "n,n,n"))
+                         (const_int 1)
+                         (match_operand 1 "const_int_operand"              "n,n,n"))
+        (match_operand:QI 2 "nonmemory_operand"                            "L,P,r"))]
+  "optimize
+   && IN_RANGE (INTVAL(operands[1]), 0, 7)"
+  "@
+	cbi %m0-0x20,%1
+	sbi %m0-0x20,%1
+	sbrc %2,0\;sbi %m0-0x20,%1\;sbrs %2,0\;cbi %m0-0x20,%1"
+  [(set_attr "length" "1,1,4")
+   (set_attr "cc" "none")])

Re: [Patch,AVR]: Solve PR42210

[Richard Henderson]

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

On 04/21/2011 05:31 AM, Georg-Johann Lay wrote:
> +;; Some combiner patterns dealing with bits.
> +;; See PR42210
> +
> +;; Move bit $3.$4 into bit $0.$4
> +(define_insn "*movbitqi.1-6.a"
...
> +(define_insn "*movbitqi.1-6.b"
...
> +(define_insn "*movbitqi.0"
...
> +(define_insn "*movbitqi.7"

This looks to be way more complicated than necessary.  Consider implementing the
extzv and insv named patterns, and using zero_extract properly.

E.g. the following, lightly tested.  Looking again at the comment on your last
pattern, it looks like that one would still need to be included.


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 3264 bytes --]

diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md
index 1ab3033..e557fcd 100644
--- a/gcc/config/avr/avr.md
+++ b/gcc/config/avr/avr.md
@@ -3388,3 +3388,74 @@
 	clr __zero_reg__"
   [(set_attr "length" "3")
    (set_attr "cc" "clobber")])
+
+(define_expand "extzv"
+  [(set (match_operand:QI 0 "register_operand" "")
+	(zero_extract:QI
+	  (match_operand:QI 1 "register_operand" "")
+	  (match_operand:QI 2 "const1_operand" "")
+	  (match_operand:QI 3 "const_0_to_7_operand" "")))]
+  ""
+  "")
+
+(define_expand "insv"
+  [(set (zero_extract:QI
+	  (match_operand:QI 0 "register_operand" "")
+	  (match_operand:QI 1 "const1_operand" "")
+	  (match_operand:QI 2 "const_0_to_7_operand" ""))
+	(match_operand:QI 3 "nonmemory_operand" ""))]
+  ""
+  "")
+
+(define_insn "*movbitqi_reg_ext"
+  [(set (match_operand:QI 0 "register_operand" "=r")
+	(zero_extract:QI
+	  (match_operand:QI 1 "register_operand" "r")
+	  (const_int 1)
+	  (match_operand 2 "const_0_to_7_operand" "")))]
+  ""
+  "bst %1,%2\;bld %0,0"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+(define_insn "*movbitqi_ext_ext"
+  [(set (zero_extract:QI
+	  (match_operand:QI 0 "register_operand" "+r")
+	  (const_int 1)
+	  (match_operand 1 "const_0_to_7_operand" ""))
+	(zero_extract:QI
+	  (match_operand:QI 2 "register_operand" "r")
+	  (const_int 1)
+	  (match_operand 3 "const_0_to_7_operand" "")))]
+  ""
+  "bst %2,%3\;bld %0,%1"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+(define_insn "*movbitqi_ext_reg"
+  [(set (zero_extract:QI
+	  (match_operand:QI 0 "register_operand" "+*d,*d,r,r,r")
+	  (const_int 1)
+	  (match_operand 1 "const_0_to_7_operand" ""))
+	(match_operand:QI 2 "nonmemory_operand" "L,P,L,P,r"))]
+  ""
+{
+  switch (which_alternative)
+    {
+    case 0:
+      operands[2] = gen_int_mode (~(1 << INTVAL (operands[1])), QImode);
+      return "andi %0,%2";
+    case 1:
+      operands[2] = gen_int_mode (1 << INTVAL (operands[1]), QImode);
+      return "ori %0,%2";
+    case 2:
+      return "clt\;bld %0,%1";
+    case 3:
+      return "set\;bld %0,%1";
+    case 4:
+      return "bst %2,0\;bld %0,%1";
+    }
+  gcc_unreachable ();
+}
+  [(set_attr "length" "1,1,2,2,2")
+   (set_attr "cc" "set_zn,set_zn,none,none,none")])
diff --git a/gcc/config/avr/predicates.md b/gcc/config/avr/predicates.md
index 9a3473b..e0142d1 100755
--- a/gcc/config/avr/predicates.md
+++ b/gcc/config/avr/predicates.md
@@ -62,6 +62,11 @@
   (and (match_code "const_int,const_double")
        (match_test "op == CONST0_RTX (mode)")))
 
+;; Return 1 if OP is the one constant for MODE.
+(define_predicate "const1_operand"
+  (and (match_code "const_int")
+       (match_test "op == CONST1_RTX (mode)")))
+
 ;; Returns true if OP is either the constant zero or a register.
 (define_predicate "reg_or_0_operand"
   (ior (match_operand 0 "register_operand")
@@ -106,6 +111,11 @@
   (and (match_code "const_int")
        (match_test "exact_log2(~INTVAL (op) & GET_MODE_MASK (mode)) >= 0")))
 
+;; Return true if OP is a constant between 0 and 7.
+(define_predicate "const_0_to_7_operand"
+  (and (match_code "const_int")
+       (match_test "IN_RANGE (INTVAL (op), 0, 7)")))
+
 ;;
 (define_predicate "avr_sp_immediate_operand"
   (and (match_code "const_int")

Re: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

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

Richard Henderson schrieb:
> On 04/21/2011 05:31 AM, Georg-Johann Lay wrote:
>> +;; Some combiner patterns dealing with bits.
>> +;; See PR42210
>> +
>> +;; Move bit $3.$4 into bit $0.$4
>> +(define_insn "*movbitqi.1-6.a"
> ...
>> +(define_insn "*movbitqi.1-6.b"
> ...
>> +(define_insn "*movbitqi.0"
> ...
>> +(define_insn "*movbitqi.7"
> 
> This looks to be way more complicated than necessary.  Consider implementing the
> extzv and insv named patterns, and using zero_extract properly.

Hi, I added some more test cases, see

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42210#c2

The case of setting bits in bitfield is easy, but for many AVRs there
is no header that defines SFRs via bitfields: there are just defines
for SFR address and bit position so that user code often contains bulk
of AND/OR/SHIFT/NOT etc. As the SFRs are volatile, insv expander skips
them and a combine pattern must care of them. Omitting the complicated
bit-plethora patterns I still see long, slow shift loops for HI. So
the patch still includes them.

> E.g. the following, lightly tested.  Looking again at the comment on your last
> pattern, it looks like that one would still need to be included.

Some lines are longer than 79 chars; I prefered legibility as there
are already long lines in avr.md, anyway.
The snip "INTVAL(operands[...])" occurs more than 50 times in the
machine description. Wouln't it be good to have an abbreviation like
#define OPVAL(n) INTVAL(operands[n])) defined in, e.g. avr-protos.h?

> +(define_insn "*movbitqi_ext_reg"
> +  [(set (zero_extract:QI
> +	  (match_operand:QI 0 "register_operand" "+*d,*d,r,r,r")
> +	  (const_int 1)
> +	  (match_operand 1 "const_0_to_7_operand" ""))
> +	(match_operand:QI 2 "nonme

Why do you use "*d" as constraint and not "d"?

> r~

Johann

	PR target/42210

	* config/avr/predicates.md (const1_operand, const_0_to_7_operand):
	New predicates.
	
	* config/avr/avr.md ("insv"): New insn expander.
	("*movbitqi.1-6.a", "*movbitqi.1-6.b", "*movbitqi.0", "*insv.io",
	"*insv.not.io", "*insv.reg"): New insns.



[-- Attachment #2: pr42210.diff --]
[-- Type: text/x-patch, Size: 6286 bytes --]

Index: config/avr/predicates.md
===================================================================
--- config/avr/predicates.md	(Revision 172902)
+++ config/avr/predicates.md	(Arbeitskopie)
@@ -62,6 +62,17 @@ (define_predicate "const0_operand"
   (and (match_code "const_int,const_double")
        (match_test "op == CONST0_RTX (mode)")))
 
+;; Return 1 if OP is the one constant integer for MODE.
+(define_predicate "const1_operand"
+  (and (match_code "const_int")
+       (match_test "op == CONST1_RTX (mode)")))
+
+
+;; Return 1 if OP is constant integer 0..7 for MODE.
+(define_predicate "const_0_to_7_operand"
+  (and (match_code "const_int")
+       (match_test "IN_RANGE (INTVAL (op), 0, 7)")))
+
 ;; Returns true if OP is either the constant zero or a register.
 (define_predicate "reg_or_0_operand"
   (ior (match_operand 0 "register_operand")
Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(Revision 172902)
+++ config/avr/avr.md	(Arbeitskopie)
@@ -3388,3 +3388,119 @@ (define_insn "fmulsu"
 	clr __zero_reg__"
   [(set_attr "length" "3")
    (set_attr "cc" "clobber")])
+
+\f
+;; Some combiner patterns dealing with bits.
+;; See PR42210
+
+;; Move bit $3.$4 into bit $0.$4
+(define_insn "*movbitqi.1-6.a"
+  [(set (match_operand:QI 0 "register_operand"                               "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"                "0")
+                        (match_operand:QI 2 "single_zero_operand"             "n"))
+                (and:QI (ashift:QI (match_operand:QI 3 "register_operand"     "r")
+                                   (match_operand:QI 4 "const_0_to_7_operand" "n"))
+                        (match_operand:QI 5 "single_one_operand"              "n"))))]
+  "optimize
+   && INTVAL(operands[4]) == exact_log2 (INTVAL(operands[5]) & GET_MODE_MASK (QImode))
+   && INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.$4
+;; Variation of above. Unfortunately, there is no canonicalized representation
+;; of moving around bits.  So what we see here depends on how user writes down
+;; bit manipulations.
+(define_insn "*movbitqi.1-6.b"
+  [(set (match_operand:QI 0 "register_operand"                            "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"             "0")
+                        (match_operand:QI 2 "single_zero_operand"          "n"))
+                (ashift:QI (and:QI (match_operand:QI 3 "register_operand"  "r")
+                                   (const_int 1))
+                           (match_operand:QI 4 "const_0_to_7_operand"      "n"))))]
+  "optimize
+   && INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.0.
+;; For bit 0, combiner generates slightly different pattern.
+(define_insn "*movbitqi.0"
+  [(set (match_operand:QI 0 "register_operand"                     "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"      "0")
+                        (match_operand:QI 2 "single_zero_operand"   "n"))
+                (and:QI (match_operand:QI 3 "register_operand"      "r")
+                        (const_int 1))))]
+  "optimize
+   && 0 == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,0"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $2.0 into bit $0.7.
+;; For bit 7, combiner generates slightly different pattern
+(define_insn "*movbitqi.7"
+  [(set (match_operand:QI 0 "register_operand"                      "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"       "0")
+                        (const_int 127))
+                (ashift:QI (match_operand:QI 2 "register_operand"    "r")
+                           (const_int 7))))]
+  "optimize"
+  "bst %2,0\;bld %0,7"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Combiner transforms above four pattern into ZERO_EXTRACT if it sees MEM
+;; and input/output match.  We provide a special pattern for this, because
+;; in contrast to a IN/BST/BLD/OUT sequence we need less registers and the
+;; operation on I/O is atomic.
+(define_insn "*insv.io"
+  [(set (zero_extract:QI (mem:QI (match_operand 0 "low_io_address_operand" "n,n,n"))
+                         (const_int 1)
+                         (match_operand:QI 1 "const_0_to_7_operand"        "n,n,n"))
+        (match_operand:QI 2 "nonmemory_operand"                            "L,P,r"))]
+  "optimize"
+  "@
+	cbi %m0-0x20,%1
+	sbi %m0-0x20,%1
+	sbrc %2,0\;sbi %m0-0x20,%1\;sbrs %2,0\;cbi %m0-0x20,%1"
+  [(set_attr "length" "1,1,4")
+   (set_attr "cc" "none")])
+
+(define_insn "*insv.not.io"
+  [(set (zero_extract:QI (mem:QI (match_operand 0 "low_io_address_operand" "n"))
+                         (const_int 1)
+                         (match_operand:QI 1 "const_0_to_7_operand"        "n"))
+        (not:QI (match_operand:QI 2 "register_operand"                     "r")))]
+  "optimize"
+  "sbrs %2,0\;sbi %m0-0x20,%1\;sbrc %2,0\;cbi %m0-0x20,%1"
+  [(set_attr "length" "4")
+   (set_attr "cc" "none")])
+
+;; The insv expander.
+;; We only support 1-bit inserts
+(define_expand "insv"
+  [(set (zero_extract:QI (match_operand:QI 0 "register_operand" "")
+                         (match_operand:QI 1 "const1_operand" "")        ; width
+                         (match_operand:QI 2 "const_0_to_7_operand" "")) ; pos
+        (match_operand:QI 3 "nonmemory_operand" ""))]
+  "optimize"
+  "")
+
+;; Insert bit $2.0 into $0.$1
+(define_insn "*insv.reg"
+  [(set (zero_extract:QI (match_operand:QI 0 "register_operand"    "+r,d,d,l,l")
+                         (const_int 1)
+                         (match_operand:QI 1 "const_0_to_7_operand" "n,n,n,n,n"))
+        (match_operand:QI 2 "nonmemory_operand"                     "r,L,P,L,P"))]
+  "optimize"
+  "@
+	bst %2,0\;bld %0,%1
+	andi %0,lo8(~(1<<%1))
+	ori %0,lo8(1<<%1)
+	clt\;bld %0,%1
+	set\;bld %0,%1"
+  [(set_attr "length" "2,1,1,2,2")
+   (set_attr "cc" "none,set_zn,set_zn,none,none")])

Re: [Patch,AVR]: Solve PR42210

[Richard Henderson]

On 04/26/2011 04:51 AM, Georg-Johann Lay wrote:
> As the SFRs are volatile, insv expander skips
> them and a combine pattern must care of them. Omitting the complicated
> bit-plethora patterns I still see long, slow shift loops for HI. So
> the patch still includes them.

Why don't we focus on doing the right thing for plain registers first,
and worry about volatile SFRs second?

In particular, combine *should* be doing the right thing for registers,
putting those AND/OR/SHIFT whatsits back into insv/extv form, via
make_compound_operation.

> The snip "INTVAL(operands[...])" occurs more than 50 times in the
> machine description. Wouln't it be good to have an abbreviation like
> #define OPVAL(n) INTVAL(operands[n])) defined in, e.g. avr-protos.h?

No, what would be better is to define more predicates.  Though in the
case of comparing op2 vs op4, there's probably no helping it.

>> +(define_insn "*movbitqi_ext_reg"
>> +  [(set (zero_extract:QI
>> +	  (match_operand:QI 0 "register_operand" "+*d,*d,r,r,r")
>> +	  (const_int 1)
>> +	  (match_operand 1 "const_0_to_7_operand" ""))
>> +	(match_operand:QI 2 "nonme
> 
> Why do you use "*d" as constraint and not "d"?

Premature optimization.  But what it means is ignore "d" as a register
allocation class and only consider "r".

Why are you adding "optimize" to all these insns?  None of them will
be matched unless combine is run, which implies optimization.


r~

Re: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

Richard Henderson schrieb:
> On 04/26/2011 04:51 AM, Georg-Johann Lay wrote:
>> As the SFRs are volatile, insv expander skips
>> them and a combine pattern must care of them. Omitting the complicated
>> bit-plethora patterns I still see long, slow shift loops for HI. So
>> the patch still includes them.
> 
> Why don't we focus on doing the right thing for plain registers first,
> and worry about volatile SFRs second?

The shift loops occur on registers. The insv now covers registers.
Besides that, in many places these operations are actually needed to
set SFR bits. I see combine try many insv for HImode; I don't know if
it is wise to support them without actually knowing what they are used
for. Most of this stuff will actually just use one byte of the values,
but that can't be seen at that point.

> In particular, combine *should* be doing the right thing for registers,
> putting those AND/OR/SHIFT whatsits back into insv/extv form, via
> make_compound_operation.

Yes it should. Bit combine only looks into thins up to some depth, and
intermediate patterns can help combine find the way. In the particular
case the combine bridges do no harm because they map to efficient
instruction sequences in all cases.

>> The snip "INTVAL(operands[...])" occurs more than 50 times in the
>> machine description. Wouln't it be good to have an abbreviation like
>> #define OPVAL(n) INTVAL(operands[n])) defined in, e.g. avr-protos.h?
> 
> No, what would be better is to define more predicates.  Though in the
> case of comparing op2 vs op4, there's probably no helping it.

Was just some a suggestion. I don't see how predicate would help in
switch (INTVAL(operands[n])) or similar places in all the C snipps...
The predicates you indicated are already included in the last patch.

> Why are you adding "optimize" to all these insns?  None of them will
> be matched unless combine is run, which implies optimization.

Similar pattern like *sbi, *cbi etc use that insn condition. I don't
know what the rationale behind that is/was, I just followed that.

Johann

Re: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

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

Richard Henderson schrieb:

> Why are you adding "optimize" to all these insns?  None of them will
> be matched unless combine is run, which implies optimization.

Here is a patch without optimize in the insn conditions.

The optimize condition is still present in the insv expander because I
do not know what the policy about that is in the avr backend.

Johann

> 
> r~
> 

	PR target/42210

	* config/avr/predicates.md (const1_operand, const_0_to_7_operand):
	New predicates.
	
	* config/avr/avr.md ("insv"): New insn expander.
	("*movbitqi.1-6.a", "*movbitqi.1-6.b", "*movbitqi.0", "*insv.io",
	"*insv.not.io", "*insv.reg"): New insns.

[-- Attachment #2: pr42210.diff --]
[-- Type: text/x-patch, Size: 6206 bytes --]

Index: config/avr/predicates.md
===================================================================
--- config/avr/predicates.md	(Revision 172902)
+++ config/avr/predicates.md	(Arbeitskopie)
@@ -62,6 +62,17 @@ (define_predicate "const0_operand"
   (and (match_code "const_int,const_double")
        (match_test "op == CONST0_RTX (mode)")))
 
+;; Return 1 if OP is the one constant integer for MODE.
+(define_predicate "const1_operand"
+  (and (match_code "const_int")
+       (match_test "op == CONST1_RTX (mode)")))
+
+
+;; Return 1 if OP is constant integer 0..7 for MODE.
+(define_predicate "const_0_to_7_operand"
+  (and (match_code "const_int")
+       (match_test "IN_RANGE (INTVAL (op), 0, 7)")))
+
 ;; Returns true if OP is either the constant zero or a register.
 (define_predicate "reg_or_0_operand"
   (ior (match_operand 0 "register_operand")
Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(Revision 172902)
+++ config/avr/avr.md	(Arbeitskopie)
@@ -3388,3 +3388,116 @@ (define_insn "fmulsu"
 	clr __zero_reg__"
   [(set_attr "length" "3")
    (set_attr "cc" "clobber")])
+
+\f
+;; Some combiner patterns dealing with bits.
+;; See PR42210
+
+;; Move bit $3.$4 into bit $0.$4
+(define_insn "*movbitqi.1-6.a"
+  [(set (match_operand:QI 0 "register_operand"                               "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"                "0")
+                        (match_operand:QI 2 "single_zero_operand"             "n"))
+                (and:QI (ashift:QI (match_operand:QI 3 "register_operand"     "r")
+                                   (match_operand:QI 4 "const_0_to_7_operand" "n"))
+                        (match_operand:QI 5 "single_one_operand"              "n"))))]
+  "INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))
+   && INTVAL(operands[4]) == exact_log2 (INTVAL(operands[5]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.$4
+;; Variation of above. Unfortunately, there is no canonicalized representation
+;; of moving around bits.  So what we see here depends on how user writes down
+;; bit manipulations.
+(define_insn "*movbitqi.1-6.b"
+  [(set (match_operand:QI 0 "register_operand"                            "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"             "0")
+                        (match_operand:QI 2 "single_zero_operand"          "n"))
+                (ashift:QI (and:QI (match_operand:QI 3 "register_operand"  "r")
+                                   (const_int 1))
+                           (match_operand:QI 4 "const_0_to_7_operand"      "n"))))]
+  "INTVAL(operands[4]) == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,%4"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $3.0 into bit $0.0.
+;; For bit 0, combiner generates slightly different pattern.
+(define_insn "*movbitqi.0"
+  [(set (match_operand:QI 0 "register_operand"                     "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"      "0")
+                        (match_operand:QI 2 "single_zero_operand"   "n"))
+                (and:QI (match_operand:QI 3 "register_operand"      "r")
+                        (const_int 1))))]
+  "0 == exact_log2 (~INTVAL(operands[2]) & GET_MODE_MASK (QImode))"
+  "bst %3,0\;bld %0,0"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Move bit $2.0 into bit $0.7.
+;; For bit 7, combiner generates slightly different pattern
+(define_insn "*movbitqi.7"
+  [(set (match_operand:QI 0 "register_operand"                      "=r")
+        (ior:QI (and:QI (match_operand:QI 1 "register_operand"       "0")
+                        (const_int 127))
+                (ashift:QI (match_operand:QI 2 "register_operand"    "r")
+                           (const_int 7))))]
+  ""
+  "bst %2,0\;bld %0,7"
+  [(set_attr "length" "2")
+   (set_attr "cc" "none")])
+
+;; Combiner transforms above four pattern into ZERO_EXTRACT if it sees MEM
+;; and input/output match.  We provide a special pattern for this, because
+;; in contrast to a IN/BST/BLD/OUT sequence we need less registers and the
+;; operation on I/O is atomic.
+(define_insn "*insv.io"
+  [(set (zero_extract:QI (mem:QI (match_operand 0 "low_io_address_operand" "n,n,n"))
+                         (const_int 1)
+                         (match_operand:QI 1 "const_0_to_7_operand"        "n,n,n"))
+        (match_operand:QI 2 "nonmemory_operand"                            "L,P,r"))]
+  ""
+  "@
+	cbi %m0-0x20,%1
+	sbi %m0-0x20,%1
+	sbrc %2,0\;sbi %m0-0x20,%1\;sbrs %2,0\;cbi %m0-0x20,%1"
+  [(set_attr "length" "1,1,4")
+   (set_attr "cc" "none")])
+
+(define_insn "*insv.not.io"
+  [(set (zero_extract:QI (mem:QI (match_operand 0 "low_io_address_operand" "n"))
+                         (const_int 1)
+                         (match_operand:QI 1 "const_0_to_7_operand"        "n"))
+        (not:QI (match_operand:QI 2 "register_operand"                     "r")))]
+  ""
+  "sbrs %2,0\;sbi %m0-0x20,%1\;sbrc %2,0\;cbi %m0-0x20,%1"
+  [(set_attr "length" "4")
+   (set_attr "cc" "none")])
+
+;; The insv expander.
+;; We only support 1-bit inserts
+(define_expand "insv"
+  [(set (zero_extract:QI (match_operand:QI 0 "register_operand" "")
+                         (match_operand:QI 1 "const1_operand" "")        ; width
+                         (match_operand:QI 2 "const_0_to_7_operand" "")) ; pos
+        (match_operand:QI 3 "nonmemory_operand" ""))]
+  "optimize"
+  "")
+
+;; Insert bit $2.0 into $0.$1
+(define_insn "*insv.reg"
+  [(set (zero_extract:QI (match_operand:QI 0 "register_operand"    "+r,d,d,l,l")
+                         (const_int 1)
+                         (match_operand:QI 1 "const_0_to_7_operand" "n,n,n,n,n"))
+        (match_operand:QI 2 "nonmemory_operand"                     "r,L,P,L,P"))]
+  ""
+  "@
+	bst %2,0\;bld %0,%1
+	andi %0,lo8(~(1<<%1))
+	ori %0,lo8(1<<%1)
+	clt\;bld %0,%1
+	set\;bld %0,%1"
+  [(set_attr "length" "2,1,1,2,2")
+   (set_attr "cc" "none,set_zn,set_zn,none,none")])

Ping #1: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

Georg-Johann Lay schrieb:
> Richard Henderson schrieb:
> 
>> Why are you adding "optimize" to all these insns?  None of them will
>> be matched unless combine is run, which implies optimization.
> 
> Here is a patch without optimize in the insn conditions.
> 
> The optimize condition is still present in the insv expander because I
> do not know what the policy about that is in the avr backend.
> 
> Johann
> 
>> r~
>>
> 
> 	PR target/42210
> 
> 	* config/avr/predicates.md (const1_operand, const_0_to_7_operand):
> 	New predicates.
> 	
> 	* config/avr/avr.md ("insv"): New insn expander.
> 	("*movbitqi.1-6.a", "*movbitqi.1-6.b", "*movbitqi.0", "*insv.io",
> 	"*insv.not.io", "*insv.reg"): New insns.
> 

Re: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

Georg-Johann Lay schrieb:
> Richard Henderson schrieb:
> 
>> Why are you adding "optimize" to all these insns?  None of them will
>> be matched unless combine is run, which implies optimization.
> 
> Here is a patch without optimize in the insn conditions.
> 
> The optimize condition is still present in the insv expander because I
> do not know what the policy about that is in the avr backend.
> 
> Johann
> 
>> r~
>>
> 
> 	PR target/42210
> 
> 	* config/avr/predicates.md (const1_operand, const_0_to_7_operand):
> 	New predicates.
> 	
> 	* config/avr/avr.md ("insv"): New insn expander.
> 	("*movbitqi.1-6.a", "*movbitqi.1-6.b", "*movbitqi.0", "*insv.io",
> 	"*insv.not.io", "*insv.reg"): New insns.
> 

Patch:
http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02099.html

As Denis wrote in
http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00081.html
some pattern are too hard to understand. Presumably that are the
patches that open-code some bit insertions.

Would the path, be ok without these patterns, namely without

"*movbitqi.1-6.a"
"*movbitqi.1-6.b"
"*movbitqi.0"
"*movbitqi.7"

A am still of the opinion that avr should make use of insn combine and
it's power. If these patterns are too hard to understand I can add
some more comments and explanations.

The insv-like patterns are plain vanilla

"insv"
"*insv.io"
"*insv.reg"

These patterns are likely on machines that come with bitfield
representations for SFRs. On targets where just SFR addresses and bit
positions are available, bit insertions and extractions are open coded
by bunch of or-and-not-mask arithmetic, so it's not unlikely to see
them in the insn stream. Moreover, RTL lowering won't use insv on
volatile mem, just combiner can do that magic. Without the patch there
are places where a long, slow 16-bit(!) shift is used requiring a
bulky slow loop.

If you sag what patterns are ok an which are not, I will make an
according patch. (Without combine bridges other patterns might be
dead, because combine doesn't look deep enough).

Johann

Re: [Patch,AVR]: Solve PR42210

[Georg-Johann Lay]

Georg-Johann Lay wrote:
> Georg-Johann Lay wrote:
> 
>>Richard Henderson wrote:
>>
>>
>>>Why are you adding "optimize" to all these insns?  None of them will
>>>be matched unless combine is run, which implies optimization.
>>
>>Here is a patch without optimize in the insn conditions.
>>
>>The optimize condition is still present in the insv expander because I
>>do not know what the policy about that is in the avr backend.

http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02099.html

Hi, I still have a patch hanging around and don't know what to do with 
it. Install, change (and if, how) or just throw away.


Thanks, Johann

>>	PR target/42210
>>
>>	* config/avr/predicates.md (const1_operand, const_0_to_7_operand):
>>	New predicates.
>>	
>>	* config/avr/avr.md ("insv"): New insn expander.
>>	("*movbitqi.1-6.a", "*movbitqi.1-6.b", "*movbitqi.0", "*insv.io",
>>	"*insv.not.io", "*insv.reg"): New insns.
>>
> 
> 
> Patch:
> http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02099.html
> 
> As Denis wrote in
> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00081.html
> some pattern are too hard to understand. Presumably that are the
> patches that open-code some bit insertions.
> 
> Would the path, be ok without these patterns, namely without
> 
> "*movbitqi.1-6.a"
> "*movbitqi.1-6.b"
> "*movbitqi.0"
> "*movbitqi.7"
> 
> A am still of the opinion that avr should make use of insn combine and
> it's power. If these patterns are too hard to understand I can add
> some more comments and explanations.
> 
> The insv-like patterns are plain vanilla
> 
> "insv"
> "*insv.io"
> "*insv.reg"
> 
> These patterns are likely on machines that come with bitfield
> representations for SFRs. On targets where just SFR addresses and bit
> positions are available, bit insertions and extractions are open coded
> by bunch of or-and-not-mask arithmetic, so it's not unlikely to see
> them in the insn stream. Moreover, RTL lowering won't use insv on
> volatile mem, just combiner can do that magic. Without the patch there
> are places where a long, slow 16-bit(!) shift is used requiring a
> bulky slow loop.
> 
> If you sag what patterns are ok an which are not, I will make an
> according patch. (Without combine bridges other patterns might be
> dead, because combine doesn't look deep enough).
> 
> Johann

Re: [Patch,AVR]: Solve PR42210

[Denis Chertykov]

2011/5/28 Georg-Johann Lay <avr@gjlay.de>:
> Georg-Johann Lay wrote:
>>
>> Georg-Johann Lay wrote:
>>
>>> Richard Henderson wrote:
>>>
>>>
>>>> Why are you adding "optimize" to all these insns?  None of them will
>>>> be matched unless combine is run, which implies optimization.
>>>
>>> Here is a patch without optimize in the insn conditions.
>>>
>>> The optimize condition is still present in the insv expander because I
>>> do not know what the policy about that is in the avr backend.
>
> http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02099.html
>
> Hi, I still have a patch hanging around and don't know what to do with it.
> Install, change (and if, how) or just throw away.
>

Please, commit it.

Denis.