#2543 [PIC16] BANKSEL generation incorrect, useless MOVFFs generated
Bug reported by Kustaa Nyholm on sdcc-user on Aug 29, 2016:
void testxxx() {
hid_tx_buffer.uint8[33] = g_stepper_state[1].position.uint8[1];
hid_tx_buffer.uint8[34] = g_stepper_state[1].position.uint8[2];
}
The first assignment works but the second one fails (based on observing
the results in actual hardware, PIC18F45K50).
Looking at the compiled code it looks to me that there is a missing BANKSEL
at line 01021?
Also I don't see the point of the MOVFF instructions at 01013 and 01019,
what is the purpose?
000000 01008 _testxxx:
01009 ; .line 59; main.c hid_tx_buffer.uint8[34] = g_stepper_state[1].position.uint8[2];
000000 CFD9 FFE5 01010 MOVFF FSR2L, POSTDEC1
000004 CFE1 FFD9 01011 MOVFF FSR1L, FSR2L
01012 ; .line 60; main.c }
000008 C??? F??? 01013 MOVFF (_g_stepper_state + 5), (_g_stepper_state + 5)
00000C ???? 01014 BANKSEL (_g_stepper_state + 5)
00000E 51?? 01015 MOVF (_g_stepper_state + 5), W, B
000010 ???? 01016 BANKSEL (_hid_tx_buffer + 33)
000012 6F?? 01017 MOVWF (_hid_tx_buffer + 33), B
01018 ; .line 61; main.c
000014 C??? F??? 01019 MOVFF (_g_stepper_state + 6), (_g_stepper_state + 6)
000018 51?? 01020 MOVF (_g_stepper_state + 6), W, B
01021 ; removed redundant BANKSEL
00001A 6F?? 01022 MOVWF (_hid_tx_buffer + 34), B
00001C CFE4 FFD9 01023 MOVFF PREINC1, FSR2L
000020 0012 01024 RETURN
The declarations for the data structures are as follows.
In a header file included in "main.c"
//------------------------------------
typedef union {
uint8_t uint8[4];
uint16_t uint16[2];
uint32_t uint32;
} uint32_t_multi_view;
typedef struct {
uint32_t_multi_view position;
} stepper_status_t;
extern volatile __at(0x300) stepper_status_t g_stepper_state[];
//------------------------------------
and in an other header file, also included in "main.c"
//------------------------------------
typedef struct {
union {
uint8_t uint8[64];
uint16_t uint16[32];
uint32_t uint32[16];
int8_t int8[64];
int16_t int16[32];
int32_t int32[16];
};
} hid_buffer_t;
extern volatile hid_buffer_t hid_tx_buffer;
//------------------------------------
and oh yeah, this is my SDCC on Mac OS X Yosemite:
//------------------------------------
/Users/nyholku/sdcc-3.4.0/bin/sdcc -v
SDCC : mcs51/z80/z180/r2k/r3ka/gbz80/tlcs90/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8 3.4.0 #8981 (Apr 5 2014) (Mac OS X i386)
published under GNU General Public License (GPL)
//------------------------------------
Here is a minimal self contained test case that produces the problem:
//------------------------------------------
typedef struct {
char member;
} test_type;
extern test_type g_test1[];
extern test_type g_test2;
void foobar() {
g_test2.member = g_test1[0].member;
g_test2.member = g_test1[0].member;
}
//------------------------------------------
generated code:
//------------------------------------------
01008 S_main__foobar code
000000 01009 _foobar:
01010 ; .line 66; main.c g_test2.member = g_test1[0].member;
000000 CFD9 FFE5 01011 MOVFF FSR2L, POSTDEC1
000004 CFE1 FFD9 01012 MOVFF FSR1L, FSR2L
01013 ; .line 67; main.c }
000008 C??? F??? 01014 MOVFF _g_test1, _g_test1
00000C ???? 01015 BANKSEL _g_test1
00000E 51?? 01016 MOVF _g_test1, W, B
000010 ???? 01017 BANKSEL _g_test2
000012 6F?? 01018 MOVWF _g_test2, B
01019 ; .line 68; main.c
000014 C??? F??? 01020 MOVFF _g_test1, _g_test1
000018 51?? 01021 MOVF _g_test1, W, B
01022 ; removed redundant BANKSEL
00001A 6F?? 01023 MOVWF _g_test2, B
00001C CFE4 FFD9 01024 MOVFF PREINC1, FSR2L
000020 0012 01025 RETURN
//------------------------------------------
compiled with:
//------------------------------------------
/Users/nyholku/sdcc-3.6.0/bin/sdcc --no-crt --ivt-loc=0x800 -V -L /Users/nyholku/sdcc-3.4.0/share/sdcc/non-free/lib/pic16 -Wa,-S,0 -Wl,-m,-s18f45k50.lkr -mpic16 -p18f45k50 --disable-warning 85 --std-sdcc99 --obanksel=3 --use-non-free main.c
//------------------------------------------
Thinking this through while the code would work with without the BANKSEL optimisation,
it would of course be preferable if the assignment would be optimised to a single MOVFF instruction.
And I think (without knowing the details of code generation or peephole optimisation)
that this should be easy to do because the compiler at that point knows both the source
and destination absolute (even if symbolic) addresses.
I believe I have a fix for all the reported issues:
Patch attached. If anyone wants to review it -- you are welcome. Esp. the SPIL_LOC && IS_TEMP is something I do not really understand -- it works for our situation, but I am not sure how correct / generally applicable that is :-(.
I'm not familiar with the pics or the pic backends. But those two commented out lines probably should go or get some justification for why there is commented out code.
Philipp
Hi,
the "missing BANKSEL" bug is still alive in SDCC4.0.0.
IMO it should be considered as high priority; in my use case, I tried to clear a variable, but another one was cleared instead... I could verify in the assembly listing that a BANKSEL was missing after the variable has been used by MOVFF.
So I tried Raphael's fix, and it actually fixed the "missing BANKSEL" bug but seemed to bring new ones, which I didn't take the time to investigate, but anyway my firmware missed some functionalities (but a part of the program was working; it's quite big PIC18F26K22 firmware).
Then I removed from his patch the "pic16/gen.c" part, and my firmware was working again!
I'm not at all an expert in compiler writing, so I'm not able to judge the quality of the fix, but all I can say is that the "pic16/pcode.c" part... saves my life.
Should I create a new ticket with a patch containing only the part that works fine here?
The pic ports are unmaintained. SDCC developers might be able to test and commit patches with fixes, when provided. They'll be reluctatn to commit larger, harder-to-understand patches.
Long-term the only hope for the pic ports is someone volunteering to maintain them.