Bug 42210 - avr: optimizing assignment to a bit field
Summary: avr: optimizing assignment to a bit field
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.3.3
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2009-11-29 01:05 UTC by Shaun Jackman
Modified: 2011-06-06 09:10 UTC (History)
4 users (show)

See Also:
Host:
Target: avr-unknown-none
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-11-29 14:35:09

Attachments
extended testcase (392 bytes, text/plain)
2011-04-26 11:35 UTC, Georg-Johann Lay 		Details
Patch against 4.7.0 (r173832) (1.55 KB, patch)
2011-05-17 19:16 UTC, Georg-Johann Lay 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.