Bug 42210

Summary: avr: optimizing assignment to a bit field
Product: gcc Reporter: Shaun Jackman <sjackman>
Component: targetAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: enhancement CC: eric.weddington, gcc-bugs, gjl, hutchinsonandy
Priority: P3 Keywords: missed-optimization
Version: 4.3.3   
Target Milestone: ---   
Host: Target: avr-unknown-none
Build: Known to work:
Known to fail: Last reconfirmed: 2009-11-29 14:35:09
Attachments: extended testcase
Patch against 4.7.0 (r173832)

Description Shaun Jackman 2009-11-29 01:05:39 UTC
When assigning a bool to a single bit of a bitfield located in the
bit-addressable region of memory, better code is produced by
       if (flag)
               bitfield.bit = true;
       else
               bitfield.bit = false;
than
       bitfield.bit = flag;

I've included a short test and the assembler output by both forms.
Should I file a bug suggesting a possible improvement here?

Cheers,
Shaun

#include <stdbool.h>
#include <stdint.h>

struct byte { uint8_t x0:1; uint8_t x1:1; uint8_t x2:1; uint8_t x3:1;
       uint8_t x4:1; uint8_t x5:1; uint8_t x6:1; uint8_t x7:1; };

volatile struct byte *const porte = (void*)0x23;

void set_flag_good(bool flag)
{
       if (flag)
               porte->x6 = true;
       else
               porte->x6 = false;
}

void set_flag_bad(bool flag)
{
       porte->x6 = flag;
}


00000000 <set_flag_good>:
  0:   88 23           and     r24, r24
  2:   01 f4           brne    .+0             ; 0x4 <set_flag_good+0x4>
                       2: R_AVR_7_PCREL        .text+0x8
  4:   1e 98           cbi     0x03, 6 ; 3
  6:   08 95           ret
  8:   1e 9a           sbi     0x03, 6 ; 3
  a:   08 95           ret

0000000c <set_flag_bad>:
  c:   81 70           andi    r24, 0x01       ; 1
  e:   82 95           swap    r24
 10:   88 0f           add     r24, r24
 12:   88 0f           add     r24, r24
 14:   80 7c           andi    r24, 0xC0       ; 192
 16:   93 b1           in      r25, 0x03       ; 3
 18:   9f 7b           andi    r25, 0xBF       ; 191
 1a:   98 2b           or      r25, r24
 1c:   93 b9           out     0x03, r25       ; 3
 1e:   08 95           ret
Comment 1 Andy Hutchinson 2009-11-29 14:35:09 UTC
Same on 4.5 Head.

The backend patterns match against "MEM AND singlebit". 
Combine never considers this.

Incoming RTL and Combine pass dump file extract:

;; Pred edge  ENTRY [100.0%]  (fallthru)
(note 4 0 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)

(insn 2 4 3 2 rot.c:25 (set (reg/v:QI 41 [ flag ])
        (reg:QI 24 r24 [ flag ])) 4 {*movqi} (expr_list:REG_DEAD (reg:QI 24 r24 [ flag ])
        (nil)))

(note 3 2 7 2 NOTE_INSN_FUNCTION_BEG)

(insn 7 3 8 2 rot.c:26 (set (reg:QI 43)
        (and:QI (reg/v:QI 41 [ flag ])
            (const_int 1 [0x1]))) 43 {andqi3} (expr_list:REG_DEAD (reg/v:QI 41 [ flag ])
        (nil)))

(insn 8 7 9 2 rot.c:26 (set (reg:QI 44)
        (ashift:QI (reg:QI 43)
            (const_int 6 [0x6]))) 59 {*ashlqi3} (expr_list:REG_DEAD (reg:QI 43)
        (nil)))

(insn 9 8 10 2 rot.c:26 (set (reg:QI 45)
        (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])) 4 {*movqi} (nil))

(insn 10 9 11 2 rot.c:26 (set (reg:QI 46)
        (and:QI (reg:QI 45)
            (const_int -65 [0xffffffbf]))) 43 {andqi3} (expr_list:REG_DEAD (reg:QI 45)
        (nil)))

(insn 11 10 12 2 rot.c:26 (set (reg:QI 47)
        (ior:QI (reg:QI 46)
            (reg:QI 44))) 46 {iorqi3} (expr_list:REG_DEAD (reg:QI 46)
        (expr_list:REG_DEAD (reg:QI 44)
            (nil))))

(insn 12 11 0 2 rot.c:26 (set (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
        (reg:QI 47)) 4 {*movqi} (expr_list:REG_DEAD (reg:QI 47)
        (nil)))
;; End of basic block 2 -> ( 1)




Trying 2 -> 7:
Successfully matched this instruction:
(set (reg:QI 43)
    (and:QI (reg:QI 24 r24 [ flag ])
        (const_int 1 [0x1])))
deferring deletion of insn with uid = 2.
modifying insn i3     7 r43:QI=r24:QI&0x1
      REG_DEAD: r24:QI
deferring rescan insn with uid = 7.

Trying 7 -> 8:
Failed to match this instruction:
(set (reg:QI 44)
    (and:QI (ashift:QI (reg:QI 24 r24 [ flag ])
            (const_int 6 [0x6]))
        (const_int 64 [0x40])))

Trying 9 -> 10:
Failed to match this instruction:
(set (reg:QI 46)
    (and:QI (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
        (const_int -65 [0xffffffbf])))

Trying 8 -> 11:
Failed to match this instruction:
(set (reg:QI 47)
    (ior:QI (ashift:QI (reg:QI 43)
            (const_int 6 [0x6]))
        (reg:QI 46)))

Trying 10 -> 11:
Failed to match this instruction:
(set (reg:QI 47)
    (ior:QI (and:QI (reg:QI 45)
            (const_int -65 [0xffffffbf]))
        (reg:QI 44)))

Trying 7, 8 -> 11:
Failed to match this instruction:
(set (reg:QI 47)
    (ior:QI (and:QI (ashift:QI (reg:QI 24 r24 [ flag ])
                (const_int 6 [0x6]))
            (const_int 64 [0x40]))
        (reg:QI 46)))
Failed to match this instruction:
(set (reg:QI 44)
    (and:QI (ashift:QI (reg:QI 24 r24 [ flag ])
            (const_int 6 [0x6]))
        (const_int 64 [0x40])))

Trying 9, 10 -> 11:
Failed to match this instruction:
(set (reg:QI 47)
    (ior:QI (and:QI (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
            (const_int -65 [0xffffffbf]))
        (reg:QI 44)))
Failed to match this instruction:
(set (reg:QI 46)
    (and:QI (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
        (const_int -65 [0xffffffbf])))

Trying 10, 8 -> 11:
Failed to match this instruction:
(set (reg:QI 47)
    (ior:QI (and:QI (reg:QI 45)
            (const_int -65 [0xffffffbf]))
        (ashift:QI (reg:QI 43)
            (const_int 6 [0x6]))))
Successfully matched this instruction:
(set (reg:QI 46)
    (ashift:QI (reg:QI 43)
        (const_int 6 [0x6])))
Failed to match this instruction:
(set (reg:QI 47)
    (ior:QI (and:QI (reg:QI 45)
            (const_int -65 [0xffffffbf]))
        (reg:QI 46)))

Trying 11 -> 12:
Failed to match this instruction:
(set (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
    (ior:QI (reg:QI 46)
        (reg:QI 44)))

Trying 8, 11 -> 12:
Failed to match this instruction:
(set (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
    (ior:QI (ashift:QI (reg:QI 43)
            (const_int 6 [0x6]))
        (reg:QI 46)))
Successfully matched this instruction:
(set (reg:QI 47)
    (ashift:QI (reg:QI 43)
        (const_int 6 [0x6])))
Failed to match this instruction:
(set (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
    (ior:QI (reg:QI 47)
        (reg:QI 46)))

Trying 10, 11 -> 12:
Failed to match this instruction:
(set (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
    (ior:QI (and:QI (reg:QI 45)
            (const_int -65 [0xffffffbf]))
        (reg:QI 44)))
Successfully matched this instruction:
(set (reg:QI 47)
    (and:QI (reg:QI 45)
        (const_int -65 [0xffffffbf])))
Failed to match this instruction:
(set (mem/s/j:QI (const_int 35 [0x23]) [0 S1 A8])
    (ior:QI (reg:QI 47)
        (reg:QI 44)))





__SREG__ = 0x3f
__SP_H__ = 0x3e
__SP_L__ = 0x3d
__tmp_reg__ = 0
__zero_reg__ = 1
	.global __do_copy_data
	.global __do_clear_bss
	.text
.global	set_flag_good
	.type	set_flag_good, @function
set_flag_good:
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
	tst r24
	brne .L5
	cbi 35-0x20,6
	ret
.L5:
	sbi 35-0x20,6
	ret
	.size	set_flag_good, .-set_flag_good
.global	set_flag_bad
	.type	set_flag_bad, @function
set_flag_bad:
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
	andi r24,lo8(1)
	swap r24
	lsl r24
	lsl r24
	andi r24,lo8(-64)
	in r25,35-0x20
	andi r25,lo8(-65)
	or r25,r24
	out 35-0x20,r25
/* epilogue start */
	ret
	.size	set_flag_bad, .-set_flag_bad
Comment 2 Georg-Johann Lay 2011-04-26 11:35:23 UTC
Created attachment 24102 [details]
extended testcase

Added testcase with some more variations of "setting bits in I/O" theme.
Comment 3 Georg-Johann Lay 2011-05-17 19:16:45 UTC
Created attachment 24269 [details]
Patch against 4.7.0 (r173832)

See also

http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02099.html
Comment 4 Georg-Johann Lay 2011-06-06 09:00:41 UTC
Author: gjl
Date: Mon Jun  6 09:00:36 2011
New Revision: 174685

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174685
Log:
	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.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.md
    trunk/gcc/config/avr/predicates.md
Comment 5 Georg-Johann Lay 2011-06-06 09:10:23 UTC
Closed as resolved+fixed as of patch above.