#1929 Masking bits generates non-atomic code

[Anonymous]

An expression to clear a bit (or bits) in a byte in the DATA area (or an SFR) should be atomic.

// sdcc -c testcase.c

#define MASK 0x0f
unsigned char reg;

void testcase(void)
{
reg &= ~MASK;
}

should generate:

anl _reg,#0xFe

but actually generates:

mov r7,_reg
anl ar7,#0xF0
mov _reg,r7

Using SDCC : mcs51 3.1.0 #7066 (Nov 28 2011) (Mac OS X x86_64)

Suggestion: Could this be fixed by a peeophole rule?

[Anonymous]

Test case

[Anonymous]

On checking the instruction set reference (http://www.win.tue.nl/~aeb/comp/8051/set8051.html#51anl), I may have this wrong as anl only suppords an indirect address
as the first operand. However, I would then expect SDCC to generate an atomic operation if 'reg' is declared as IDATA, viz:

unsigned char __idata reg;

Instead, it generates the following:

mov r0,#_reg
mov ar7,@r0
anl ar7,#0xF0
mov r0,#_reg
mov @r0,ar7

Also, are SFR's implitly considered DATA or IDATA?

Apologies if I'm being daft here :)

[Thomas Sailer]

If you change reg &= ~MASK to reg &= (unsigned char)~MASK, then it will generate what you want, i.e. anl _reg,#0xF0

And yes, that cast shouldn't be necessary...

[Gabriele Gorla]

sdcc 3.0 compiles it as anl _reg,#0xF0 without the cast.

[Anonymous]

Note also that by using an inverse macro, avoiding the need for the ~ operator, the correct code is generated with 3.1.0:

#define IMASK 0xf0
unsigned char reg;

void testcase(void)
{
reg &= IMASK;
}

[Anonymous]

See https://sourceforge.net/projects/sdcc/forums/forum/1865/topic/5343383/index/page/1 for discussion of workaround using a peephole rule.

[Patryk]

>> Also, are SFR's implitly considered DATA or IDATA?
SFR's can be accessed only directly, i.e. as DATA.

[Ondrej Petr]

Issue is more general and survive in sdcc 3.3 - it seems to appear any time:
lvalue is __data register,
assignment operator is |=, &= or ^=,
rvalue has type other than unsigned char.

Explicit cast of rvalue to (unsigned char) seems to force compiler to generate proper code with atomic access to lvalue.

If the bug will not be solved in next release, it should be noticed in sdcc manual as users tend to expect proper atomic behaviour that is absolutely required when lvalue is 8051 port with mixed input/output!

Example:
#include "compiler_defs.h"
#include "C8051F120_defs.h"
__bit bit_variable;
void main(void)
{
P0 |= bit_variable; // non atomic sequence generated
P0 |= (unsigned char) bit_variable; // atomic sequence generated
P0 &= ~0x01; // non atomic sequence generated for 8051 port access!!! - it's a pity, as
P0 &= (unsigned char) ~0x01; // atomic sequence generated when cast to unsigned char
P0 &= (int) 0x01; // non atomic sequence generated when cast to int
P0 &= 0x01; // atomic sequence generated w/o cast to unsigned char
P0 |= (char) ~0x01; // non atomic sequence generated when cast to unsigned char
P0 |= (char) 0x01; // non atomic sequence generated when cast to char
P0 ^= (char) 0x01; // non atomic sequence generated when cast to char
}

Command line:

• C:\PROGRA~1\SDCC\bin\sdcpp.exe -nostdinc -Wall -I"c:\SiLabs\MCU\Inc" -I"D:\Dokumenty\microsource\BlahoX\pcb_HVPSU_interface\firmware\nonatomic" -obj-ext=.rel -D__SDCC_MODEL_SMALL -DSDCC_MODEL_SMALL -D__SDCC_FLOAT_REENT -DSDCC_FLOAT_REENT -D__SDCC=3_3_0 -DSDCC=330 -D__SDCC_REVISION=8604 -DSDCC_REVISION=8604 -D__SDCC_mcs51 -DSDCC_mcs51 -D__mcs51 -D__STDC_NO_COMPLEX__ -D__STDC_NO_THREADS__ -D__STDC_NO_ATOMICS__ -D__STDC_NO_VLA__ -isystem "C:\Program Files\SDCC\bin..\include\mcs51" -isystem "C:\Program Files\SDCC\bin..\include" "D:\Dokumenty\microsource\BlahoX\pcb_HVPSU_interface\firmware\nonatomic\nonatomic.c"

[Ben Shi]

Fixed in revision #9577

Now all the following tests are optimizaed into one 'anl' instruction in mcs51-small mode.

#define MASK    0x0f
unsigned char   reg = 0xaa;
signed char   regss = 0xaa;

void testcasea(void)
{
        reg &= ~MASK;
}

void testcaseb(void)
{
        reg &= MASK;
}
void testcasec(void)
{
        regss &= ~MASK;
}
void testcased(void)
{
        regss &= MASK;
}

[Ben Shi]

Regression test of revision #9577.

Summary for 'ds390': 0 failures, 9480 tests, 1780 test cases, 7405476 bytes, 914963280 ticks
Summary for 'hc08': 0 failures, 9440 tests, 1781 test cases, 2742225 bytes, 32280171 ticks
Summary for 'stm8': 0 failures, 9549 tests, 1781 test cases, 2097754 bytes, 8140388 ticks
Summary for 'ucz80': 0 failures, 9550 tests, 1781 test cases, 2562254 bytes, 13634544 ticks
Summary for 'mcs51-small': 0 failures, 7514 tests, 1780 test cases, 2432181 bytes, 225072744 ticks
Summary for 'ucr3ka': 0 failures, 9539 tests, 1781 test cases, 2480662 bytes, 12143898 ticks

Compared to revision #9575.

Summary for 'ds390': 0 failures, 9468 tests, 1779 test cases, 7416226 bytes, 915398652 ticks
Summary for 'hc08': 0 failures, 9428 tests, 1780 test cases, 2742312 bytes, 32302160 ticks
Summary for 'stm8': 0 failures, 9537 tests, 1780 test cases, 2096395 bytes, 8143571 ticks
Summary for 'ucz80': 0 failures, 9538 tests, 1780 test cases, 2565529 bytes, 13647644 ticks
Summary for 'mcs51-small': 0 failures, 7502 tests, 1779 test cases, 2430347 bytes, 224960460 ticks
Summary for 'ucr3ka': 0 failures, 9527 tests, 1780 test cases, 2482591 bytes, 11839531 ticks

We can see bytes and ticks saved in ds390, stm8, z80, and r3ka, but slight difference in mcs51-small. (considering a new test case bug-1929.c is added).