diff mbox

[AVR] : Solve PR42210

Message ID 4DB6B1CE.3060200@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay April 26, 2011, 11:51 a.m. UTC 
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.

Comments

Richard Henderson April 26, 2011, 1:58 p.m. UTC | #1 
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~
Georg-Johann Lay April 26, 2011, 2:44 p.m. UTC | #2 
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
diff mbox

Patch

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")])
+
+
+;; 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")])